[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/docs/tools/dump_format_style.py:72
 
-  subtype, napplied = re.subn(r'^std::vector<(.*)>$', r'\1', typestr)
-  if napplied == 1:
-return 'List of ' + pluralize(to_yaml_type(subtype))
+  match = re.match(r'std::vector<(.*)>$', typestr)
+  if match:

I changed this from `subn` to `match` here since it's just a simpler way of 
expressing the same thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

I've implemented BracedInitializerIndentWidth now with significantly extended 
test coverage (including checking that it is not applied *too* broadly). The 
actual implementation is just as simple as that for 
`DesignatedInitializerIndentWidth`.

> For the yaml stuff, I for one like to define everything (even it has the 
> default value), thus I'd like the -1 or something on output. But if that 
> leads to messing around with the yaml code just use what it does.

Pretty sure I would have to modify the YAML code in order to get it to output 
something when `std::optional` is set to `std::nullopt` so I have 
left it as-is.

PTAL.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 509982.
jp4a50 added a comment.

Replace DesignatedInitializerIndentWidth with BracedInitializerIndentWidth 
which applies to a broader range of initializers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4855,6 +4855,191 @@
"[5] = ee};");
 }
 
+TEST_F(FormatTest, BracedInitializerIndentWidth) {
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.BinPackArguments = true;
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BracedInitializerIndentWidth = 6;
+
+  // Non-initializing braces are unaffected by BracedInitializerIndentWidth.
+  verifyFormat("enum class {\n"
+   "  One,\n"
+   "  Two,\n"
+   "};\n",
+   Style);
+  verifyFormat("class Foo {\n"
+   "  Foo() {}\n"
+   "  void bar();\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("auto foo = [&] {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("{\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  // Non-brace initialization is unaffected by BracedInitializerIndentWidth.
+  verifyFormat("SomeClass clazz(\n"
+   "\"xx\", \"yy\",\n"
+   "\"zz\");\n",
+   Style);
+
+  // The following types of initialization are all affected by
+  // BracedInitializerIndentWidth. Aggregate initialization.
+  verifyFormat("int LongVariable[2] = {\n"
+   "  1000, 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  \"\", \"\",\n"
+   "  \"\"};\n",
+   Style);
+  // Designated initializers.
+  verifyFormat("int LongVariable[1] = {\n"
+   "  [0] = 1000, [1] = 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  .foo = \"x\",\n"
+   "  .bar = \"y\",\n"
+   "  .baz = \"z\"};\n",
+   Style);
+  // List initialization.
+  verifyFormat("SomeStruct s{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("new SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Member initializer.
+  verifyFormat("class SomeClass {\n"
+   "  SomeStruct s{\n"
+   "\"x\",\n"
+   "\"y\",\n"
+   "\"z\",\n"
+   "  };\n"
+   "};\n",
+   Style);
+  // Constructor member initializer.
+  verifyFormat("SomeClass::SomeClass : strct{\n"
+   " \"x\",\n"
+   " \"y\",\n"
+   " \"z\",\n"
+   "   } {}\n",
+   Style);
+  // Copy initialization.
+  verifyFormat("SomeStruct s = SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Copy list initialization.
+  verifyFormat("SomeStruct s = {\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Assignment operand 

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D146101#4233535 , @jp4a50 wrote:

> Broadly speaking, these include aggregate initialization and list 
> initialization (possibly direct initialization with braces too). See the 
> initialization  
> cppref article for links to all these.
>
> As such, I would propose to actually rename 
> `DesignatedInitializerIndentWidth` to `BracedInitializerIndentWidth` (but 
> open to suggestiosn on exact naming) and have it apply to all the above types 
> of initialization.
>
> What does everyone think?

+1.




Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > rymiel wrote:
> > > owenpan wrote:
> > > > jp4a50 wrote:
> > > > > HazardyKnusperkeks wrote:
> > > > > > owenpan wrote:
> > > > > > > jp4a50 wrote:
> > > > > > > > HazardyKnusperkeks wrote:
> > > > > > > > > owenpan wrote:
> > > > > > > > > > jp4a50 wrote:
> > > > > > > > > > > owenpan wrote:
> > > > > > > > > > > > owenpan wrote:
> > > > > > > > > > > > > Using -1 to mean `ContinuationIndentWidth` is 
> > > > > > > > > > > > > unnecessary and somewhat confusing IMO.
> > > > > > > > > > > > Please disregard my comment above.
> > > > > > > > > > > Just to make sure we are on the same page, does this mean 
> > > > > > > > > > > that you are happy with the approach of using `-1` as a 
> > > > > > > > > > > default value to indicate that `ContinuationIndentWidth` 
> > > > > > > > > > > should be used?
> > > > > > > > > > > 
> > > > > > > > > > > I did initially consider using `std::optional` 
> > > > > > > > > > > and using empty optional to indicate that 
> > > > > > > > > > > `ContinuationIndentWidth` should be used but I saw that 
> > > > > > > > > > > `PPIndentWidth` was using `-1` to default to using 
> > > > > > > > > > > `IndentWidth` so I followed that precedent.
> > > > > > > > > > Yep! I would prefer the `optional`, but as you pointed out, 
> > > > > > > > > > we already got `PPIndentWidth`using `-1`.
> > > > > > > > > From the C++ side I totally agree. One could use 
> > > > > > > > > `value_or()`, which would make the code much more readable.
> > > > > > > > > And just because `PPIndentWidth` is using -1 is no reason to 
> > > > > > > > > repeat that, we could just as easily change `PPIndentWidht` 
> > > > > > > > > to an optional.
> > > > > > > > > 
> > > > > > > > > But how would it look in yaml?
> > > > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > > > *explicitly* specified - it would just be the default.
> > > > > > > > 
> > > > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set 
> > > > > > > > the `std::optional` to `4` but if 
> > > > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML 
> > > > > > > > then the optional would simply not be set during parsing.
> > > > > > > > 
> > > > > > > > Of course, if we were to change `PPIndentWidth` to work that 
> > > > > > > > way too, it would technically be a breaking change because 
> > > > > > > > users may have *explicitly* specified `-1` in their YAML.
> > > > > > > > And just because `PPIndentWidth` is using -1 is no reason to 
> > > > > > > > repeat that, we could just as easily change `PPIndentWidht` to 
> > > > > > > > an optional.
> > > > > > > 
> > > > > > > We would have to deal with backward compatibility to avoid 
> > > > > > > regressions though.
> > > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > > *explicitly* specified - it would just be the default.
> > > > > > > 
> > > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > > > > `std::optional` to `4` but if 
> > > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then 
> > > > > > > the optional would simply not be set during parsing.
> > > > > > > 
> > > > > > > Of course, if we were to change `PPIndentWidth` to work that way 
> > > > > > > too, it would technically be a breaking change because users may 
> > > > > > > have *explicitly* specified `-1` in their YAML.
> > > > > > 
> > > > > > You need an explicit entry, because you need to be able to write 
> > > > > > the empty optional on `--dump-config`.
> > > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > > *explicitly* specified - it would just be the default.
> > > > > > > 
> > > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > > > > `std::optional` to `4` but if 
> > > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then 
> > > > > > > 

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D146101#4233535 , @jp4a50 wrote:

> So at the risk of adding to the number of decisions we need to come to a 
> consensus on, I was about to update the KJ style guide to explicitly call out 
> the difference in indentation for designated initializers when I realized 
> that we (both KJ code authors and clang-format contributors) should consider 
> whether users should have the option to configure other similar types of 
> indentation following opening braces.
>
> I chatted to the owner of the KJ style guide and, whilst he did not have 
> extremely strong opinions one way or another, he and I agreed that it 
> probably makes more sense for such a config option to apply to other types of 
> braced init lists.
>
> Broadly speaking, these include aggregate initialization and list 
> initialization (possibly direct initialization with braces too). See the 
> initialization  
> cppref article for links to all these.
>
> As such, I would propose to actually rename 
> `DesignatedInitializerIndentWidth` to `BracedInitializerIndentWidth` (but 
> open to suggestiosn on exact naming) and have it apply to all the above types 
> of initialization.
>
> What does everyone think?

Would be okay for me. But then I want the documentation and tests show nested 
initialization (array of struct for example).




Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

MyDeveloperDay wrote:
> rymiel wrote:
> > owenpan wrote:
> > > jp4a50 wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > owenpan wrote:
> > > > > > jp4a50 wrote:
> > > > > > > HazardyKnusperkeks wrote:
> > > > > > > > owenpan wrote:
> > > > > > > > > jp4a50 wrote:
> > > > > > > > > > owenpan wrote:
> > > > > > > > > > > owenpan wrote:
> > > > > > > > > > > > Using -1 to mean `ContinuationIndentWidth` is 
> > > > > > > > > > > > unnecessary and somewhat confusing IMO.
> > > > > > > > > > > Please disregard my comment above.
> > > > > > > > > > Just to make sure we are on the same page, does this mean 
> > > > > > > > > > that you are happy with the approach of using `-1` as a 
> > > > > > > > > > default value to indicate that `ContinuationIndentWidth` 
> > > > > > > > > > should be used?
> > > > > > > > > > 
> > > > > > > > > > I did initially consider using `std::optional` 
> > > > > > > > > > and using empty optional to indicate that 
> > > > > > > > > > `ContinuationIndentWidth` should be used but I saw that 
> > > > > > > > > > `PPIndentWidth` was using `-1` to default to using 
> > > > > > > > > > `IndentWidth` so I followed that precedent.
> > > > > > > > > Yep! I would prefer the `optional`, but as you pointed out, 
> > > > > > > > > we already got `PPIndentWidth`using `-1`.
> > > > > > > > From the C++ side I totally agree. One could use `value_or()`, 
> > > > > > > > which would make the code much more readable.
> > > > > > > > And just because `PPIndentWidth` is using -1 is no reason to 
> > > > > > > > repeat that, we could just as easily change `PPIndentWidht` to 
> > > > > > > > an optional.
> > > > > > > > 
> > > > > > > > But how would it look in yaml?
> > > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > > *explicitly* specified - it would just be the default.
> > > > > > > 
> > > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > > > > `std::optional` to `4` but if 
> > > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then 
> > > > > > > the optional would simply not be set during parsing.
> > > > > > > 
> > > > > > > Of course, if we were to change `PPIndentWidth` to work that way 
> > > > > > > too, it would technically be a breaking change because users may 
> > > > > > > have *explicitly* specified `-1` in their YAML.
> > > > > > > And just because `PPIndentWidth` is using -1 is no reason to 
> > > > > > > repeat that, we could just as easily change `PPIndentWidht` to an 
> > > > > > > optional.
> > > > > > 
> > > > > > We would have to deal with backward compatibility to avoid 
> > > > > > regressions though.
> > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > *explicitly* specified - it would just be the default.
> > > > > > 
> > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > > > `std::optional` to `4` but if 
> > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then 
> > > > > > the optional would simply not be set during parsing.
> > > > > > 
> > > > > > Of course, if we were to change `PPIndentWidth` to work 

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

So at the risk of adding to the number of decisions we need to come to a 
consensus on, I was about to update the KJ style guide to explicitly call out 
the difference in indentation for designated initializers when I realized that 
we (both KJ code authors and clang-format contributors) should consider whether 
users should have the option to configure other similar types of indentation 
following opening braces.

I chatted to the owner of the KJ style guide and, whilst he did not have 
extremely strong opinions one way or another, he and I agreed that it probably 
makes more sense for such a config option to apply to other types of braced 
init lists.

Broadly speaking, these include aggregate initialization and list 
initialization (possibly direct initialization with braces too). See the 
initialization  
cppref article for links to all these.

As such, I would propose to actually rename `DesignatedInitializerIndentWidth` 
to `BracedInitializerIndentWidth` (but open to suggestiosn on exact naming) and 
have it apply to all the above types of initialization.

What does everyone think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

rymiel wrote:
> owenpan wrote:
> > jp4a50 wrote:
> > > HazardyKnusperkeks wrote:
> > > > owenpan wrote:
> > > > > jp4a50 wrote:
> > > > > > HazardyKnusperkeks wrote:
> > > > > > > owenpan wrote:
> > > > > > > > jp4a50 wrote:
> > > > > > > > > owenpan wrote:
> > > > > > > > > > owenpan wrote:
> > > > > > > > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary 
> > > > > > > > > > > and somewhat confusing IMO.
> > > > > > > > > > Please disregard my comment above.
> > > > > > > > > Just to make sure we are on the same page, does this mean 
> > > > > > > > > that you are happy with the approach of using `-1` as a 
> > > > > > > > > default value to indicate that `ContinuationIndentWidth` 
> > > > > > > > > should be used?
> > > > > > > > > 
> > > > > > > > > I did initially consider using `std::optional` and 
> > > > > > > > > using empty optional to indicate that 
> > > > > > > > > `ContinuationIndentWidth` should be used but I saw that 
> > > > > > > > > `PPIndentWidth` was using `-1` to default to using 
> > > > > > > > > `IndentWidth` so I followed that precedent.
> > > > > > > > Yep! I would prefer the `optional`, but as you pointed out, we 
> > > > > > > > already got `PPIndentWidth`using `-1`.
> > > > > > > From the C++ side I totally agree. One could use `value_or()`, 
> > > > > > > which would make the code much more readable.
> > > > > > > And just because `PPIndentWidth` is using -1 is no reason to 
> > > > > > > repeat that, we could just as easily change `PPIndentWidht` to an 
> > > > > > > optional.
> > > > > > > 
> > > > > > > But how would it look in yaml?
> > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > *explicitly* specified - it would just be the default.
> > > > > > 
> > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > > > `std::optional` to `4` but if 
> > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then 
> > > > > > the optional would simply not be set during parsing.
> > > > > > 
> > > > > > Of course, if we were to change `PPIndentWidth` to work that way 
> > > > > > too, it would technically be a breaking change because users may 
> > > > > > have *explicitly* specified `-1` in their YAML.
> > > > > > And just because `PPIndentWidth` is using -1 is no reason to repeat 
> > > > > > that, we could just as easily change `PPIndentWidht` to an optional.
> > > > > 
> > > > > We would have to deal with backward compatibility to avoid 
> > > > > regressions though.
> > > > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > > > specified - it would just be the default.
> > > > > 
> > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > > `std::optional` to `4` but if 
> > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > > > optional would simply not be set during parsing.
> > > > > 
> > > > > Of course, if we were to change `PPIndentWidth` to work that way too, 
> > > > > it would technically be a breaking change because users may have 
> > > > > *explicitly* specified `-1` in their YAML.
> > > > 
> > > > You need an explicit entry, because you need to be able to write the 
> > > > empty optional on `--dump-config`.
> > > > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > > > specified - it would just be the default.
> > > > > 
> > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > > `std::optional` to `4` but if 
> > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > > > optional would simply not be set during parsing.
> > > > > 
> > > > > Of course, if we were to change `PPIndentWidth` to work that way too, 
> > > > > it would technically be a breaking change because users may have 
> > > > > *explicitly* specified `-1` in their YAML.
> > > > 
> > > > You need an explicit entry, because you need to be able to write the 
> > > > empty optional on `--dump-config`.
> > > 
> > > It looks like the YAML IO logic just does the right thing (TM) with 
> > > `std::optional`s. When calling `IO.mapOptional()` on output, it simply 
> > > doesn't write the key out if the value is an empty optional. So I don't 
> > > think this is an issue.
> > > 
> > > As @owenpan says, though, there is still the issue of backward 
> > > compatibility with `PPIndentWidth`.
> > > 
> > > I don't feel strongly about which way to go so I'll leave it to you two 
> > > to decide!
> > > As @owenpan says, though, there is still 

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

owenpan wrote:
> jp4a50 wrote:
> > HazardyKnusperkeks wrote:
> > > owenpan wrote:
> > > > jp4a50 wrote:
> > > > > HazardyKnusperkeks wrote:
> > > > > > owenpan wrote:
> > > > > > > jp4a50 wrote:
> > > > > > > > owenpan wrote:
> > > > > > > > > owenpan wrote:
> > > > > > > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary 
> > > > > > > > > > and somewhat confusing IMO.
> > > > > > > > > Please disregard my comment above.
> > > > > > > > Just to make sure we are on the same page, does this mean that 
> > > > > > > > you are happy with the approach of using `-1` as a default 
> > > > > > > > value to indicate that `ContinuationIndentWidth` should be used?
> > > > > > > > 
> > > > > > > > I did initially consider using `std::optional` and 
> > > > > > > > using empty optional to indicate that `ContinuationIndentWidth` 
> > > > > > > > should be used but I saw that `PPIndentWidth` was using `-1` to 
> > > > > > > > default to using `IndentWidth` so I followed that precedent.
> > > > > > > Yep! I would prefer the `optional`, but as you pointed out, we 
> > > > > > > already got `PPIndentWidth`using `-1`.
> > > > > > From the C++ side I totally agree. One could use `value_or()`, 
> > > > > > which would make the code much more readable.
> > > > > > And just because `PPIndentWidth` is using -1 is no reason to repeat 
> > > > > > that, we could just as easily change `PPIndentWidht` to an optional.
> > > > > > 
> > > > > > But how would it look in yaml?
> > > > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > > > specified - it would just be the default.
> > > > > 
> > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > > `std::optional` to `4` but if 
> > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > > > optional would simply not be set during parsing.
> > > > > 
> > > > > Of course, if we were to change `PPIndentWidth` to work that way too, 
> > > > > it would technically be a breaking change because users may have 
> > > > > *explicitly* specified `-1` in their YAML.
> > > > > And just because `PPIndentWidth` is using -1 is no reason to repeat 
> > > > > that, we could just as easily change `PPIndentWidht` to an optional.
> > > > 
> > > > We would have to deal with backward compatibility to avoid regressions 
> > > > though.
> > > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > > specified - it would just be the default.
> > > > 
> > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > `std::optional` to `4` but if 
> > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > > optional would simply not be set during parsing.
> > > > 
> > > > Of course, if we were to change `PPIndentWidth` to work that way too, 
> > > > it would technically be a breaking change because users may have 
> > > > *explicitly* specified `-1` in their YAML.
> > > 
> > > You need an explicit entry, because you need to be able to write the 
> > > empty optional on `--dump-config`.
> > > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > > specified - it would just be the default.
> > > > 
> > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > `std::optional` to `4` but if 
> > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > > optional would simply not be set during parsing.
> > > > 
> > > > Of course, if we were to change `PPIndentWidth` to work that way too, 
> > > > it would technically be a breaking change because users may have 
> > > > *explicitly* specified `-1` in their YAML.
> > > 
> > > You need an explicit entry, because you need to be able to write the 
> > > empty optional on `--dump-config`.
> > 
> > It looks like the YAML IO logic just does the right thing (TM) with 
> > `std::optional`s. When calling `IO.mapOptional()` on output, it simply 
> > doesn't write the key out if the value is an empty optional. So I don't 
> > think this is an issue.
> > 
> > As @owenpan says, though, there is still the issue of backward 
> > compatibility with `PPIndentWidth`.
> > 
> > I don't feel strongly about which way to go so I'll leave it to you two to 
> > decide!
> > As @owenpan says, though, there is still the issue of backward 
> > compatibility with `PPIndentWidth`.
> > 
> > I don't feel strongly about which way to go so I'll leave it to you two to 
> > decide!
> 
> @MyDeveloperDay @rymiel can you weigh in?

> can you weigh in?

Well, as someone 

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

jp4a50 wrote:
> HazardyKnusperkeks wrote:
> > owenpan wrote:
> > > jp4a50 wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > owenpan wrote:
> > > > > > jp4a50 wrote:
> > > > > > > owenpan wrote:
> > > > > > > > owenpan wrote:
> > > > > > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and 
> > > > > > > > > somewhat confusing IMO.
> > > > > > > > Please disregard my comment above.
> > > > > > > Just to make sure we are on the same page, does this mean that 
> > > > > > > you are happy with the approach of using `-1` as a default value 
> > > > > > > to indicate that `ContinuationIndentWidth` should be used?
> > > > > > > 
> > > > > > > I did initially consider using `std::optional` and 
> > > > > > > using empty optional to indicate that `ContinuationIndentWidth` 
> > > > > > > should be used but I saw that `PPIndentWidth` was using `-1` to 
> > > > > > > default to using `IndentWidth` so I followed that precedent.
> > > > > > Yep! I would prefer the `optional`, but as you pointed out, we 
> > > > > > already got `PPIndentWidth`using `-1`.
> > > > > From the C++ side I totally agree. One could use `value_or()`, which 
> > > > > would make the code much more readable.
> > > > > And just because `PPIndentWidth` is using -1 is no reason to repeat 
> > > > > that, we could just as easily change `PPIndentWidht` to an optional.
> > > > > 
> > > > > But how would it look in yaml?
> > > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > > specified - it would just be the default.
> > > > 
> > > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > > `std::optional` to `4` but if 
> > > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > > optional would simply not be set during parsing.
> > > > 
> > > > Of course, if we were to change `PPIndentWidth` to work that way too, 
> > > > it would technically be a breaking change because users may have 
> > > > *explicitly* specified `-1` in their YAML.
> > > > And just because `PPIndentWidth` is using -1 is no reason to repeat 
> > > > that, we could just as easily change `PPIndentWidht` to an optional.
> > > 
> > > We would have to deal with backward compatibility to avoid regressions 
> > > though.
> > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > specified - it would just be the default.
> > > 
> > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > `std::optional` to `4` but if 
> > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > optional would simply not be set during parsing.
> > > 
> > > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > > would technically be a breaking change because users may have 
> > > *explicitly* specified `-1` in their YAML.
> > 
> > You need an explicit entry, because you need to be able to write the empty 
> > optional on `--dump-config`.
> > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > specified - it would just be the default.
> > > 
> > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > `std::optional` to `4` but if 
> > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > optional would simply not be set during parsing.
> > > 
> > > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > > would technically be a breaking change because users may have 
> > > *explicitly* specified `-1` in their YAML.
> > 
> > You need an explicit entry, because you need to be able to write the empty 
> > optional on `--dump-config`.
> 
> It looks like the YAML IO logic just does the right thing (TM) with 
> `std::optional`s. When calling `IO.mapOptional()` on output, it simply 
> doesn't write the key out if the value is an empty optional. So I don't think 
> this is an issue.
> 
> As @owenpan says, though, there is still the issue of backward compatibility 
> with `PPIndentWidth`.
> 
> I don't feel strongly about which way to go so I'll leave it to you two to 
> decide!
> As @owenpan says, though, there is still the issue of backward compatibility 
> with `PPIndentWidth`.
> 
> I don't feel strongly about which way to go so I'll leave it to you two to 
> decide!

@MyDeveloperDay @rymiel can you weigh in?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

HazardyKnusperkeks wrote:
> owenpan wrote:
> > jp4a50 wrote:
> > > HazardyKnusperkeks wrote:
> > > > owenpan wrote:
> > > > > jp4a50 wrote:
> > > > > > owenpan wrote:
> > > > > > > owenpan wrote:
> > > > > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and 
> > > > > > > > somewhat confusing IMO.
> > > > > > > Please disregard my comment above.
> > > > > > Just to make sure we are on the same page, does this mean that you 
> > > > > > are happy with the approach of using `-1` as a default value to 
> > > > > > indicate that `ContinuationIndentWidth` should be used?
> > > > > > 
> > > > > > I did initially consider using `std::optional` and using 
> > > > > > empty optional to indicate that `ContinuationIndentWidth` should be 
> > > > > > used but I saw that `PPIndentWidth` was using `-1` to default to 
> > > > > > using `IndentWidth` so I followed that precedent.
> > > > > Yep! I would prefer the `optional`, but as you pointed out, we 
> > > > > already got `PPIndentWidth`using `-1`.
> > > > From the C++ side I totally agree. One could use `value_or()`, which 
> > > > would make the code much more readable.
> > > > And just because `PPIndentWidth` is using -1 is no reason to repeat 
> > > > that, we could just as easily change `PPIndentWidht` to an optional.
> > > > 
> > > > But how would it look in yaml?
> > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > specified - it would just be the default.
> > > 
> > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > `std::optional` to `4` but if 
> > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > optional would simply not be set during parsing.
> > > 
> > > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > > would technically be a breaking change because users may have 
> > > *explicitly* specified `-1` in their YAML.
> > > And just because `PPIndentWidth` is using -1 is no reason to repeat that, 
> > > we could just as easily change `PPIndentWidht` to an optional.
> > 
> > We would have to deal with backward compatibility to avoid regressions 
> > though.
> > In YAML we wouldn't need to support empty optional being *explicitly* 
> > specified - it would just be the default.
> > 
> > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > `std::optional` to `4` but if `DesignatedInitializerIndentWidth` 
> > was omitted from the YAML then the optional would simply not be set during 
> > parsing.
> > 
> > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > would technically be a breaking change because users may have *explicitly* 
> > specified `-1` in their YAML.
> 
> You need an explicit entry, because you need to be able to write the empty 
> optional on `--dump-config`.
> > In YAML we wouldn't need to support empty optional being *explicitly* 
> > specified - it would just be the default.
> > 
> > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > `std::optional` to `4` but if `DesignatedInitializerIndentWidth` 
> > was omitted from the YAML then the optional would simply not be set during 
> > parsing.
> > 
> > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > would technically be a breaking change because users may have *explicitly* 
> > specified `-1` in their YAML.
> 
> You need an explicit entry, because you need to be able to write the empty 
> optional on `--dump-config`.

It looks like the YAML IO logic just does the right thing (TM) with 
`std::optional`s. When calling `IO.mapOptional()` on output, it simply doesn't 
write the key out if the value is an empty optional. So I don't think this is 
an issue.

As @owenpan says, though, there is still the issue of backward compatibility 
with `PPIndentWidth`.

I don't feel strongly about which way to go so I'll leave it to you two to 
decide!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

owenpan wrote:
> jp4a50 wrote:
> > HazardyKnusperkeks wrote:
> > > owenpan wrote:
> > > > jp4a50 wrote:
> > > > > owenpan wrote:
> > > > > > owenpan wrote:
> > > > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and 
> > > > > > > somewhat confusing IMO.
> > > > > > Please disregard my comment above.
> > > > > Just to make sure we are on the same page, does this mean that you 
> > > > > are happy with the approach of using `-1` as a default value to 
> > > > > indicate that `ContinuationIndentWidth` should be used?
> > > > > 
> > > > > I did initially consider using `std::optional` and using 
> > > > > empty optional to indicate that `ContinuationIndentWidth` should be 
> > > > > used but I saw that `PPIndentWidth` was using `-1` to default to 
> > > > > using `IndentWidth` so I followed that precedent.
> > > > Yep! I would prefer the `optional`, but as you pointed out, we already 
> > > > got `PPIndentWidth`using `-1`.
> > > From the C++ side I totally agree. One could use `value_or()`, which 
> > > would make the code much more readable.
> > > And just because `PPIndentWidth` is using -1 is no reason to repeat that, 
> > > we could just as easily change `PPIndentWidht` to an optional.
> > > 
> > > But how would it look in yaml?
> > In YAML we wouldn't need to support empty optional being *explicitly* 
> > specified - it would just be the default.
> > 
> > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > `std::optional` to `4` but if `DesignatedInitializerIndentWidth` 
> > was omitted from the YAML then the optional would simply not be set during 
> > parsing.
> > 
> > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > would technically be a breaking change because users may have *explicitly* 
> > specified `-1` in their YAML.
> > And just because `PPIndentWidth` is using -1 is no reason to repeat that, 
> > we could just as easily change `PPIndentWidht` to an optional.
> 
> We would have to deal with backward compatibility to avoid regressions though.
> In YAML we wouldn't need to support empty optional being *explicitly* 
> specified - it would just be the default.
> 
> So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> `std::optional` to `4` but if `DesignatedInitializerIndentWidth` 
> was omitted from the YAML then the optional would simply not be set during 
> parsing.
> 
> Of course, if we were to change `PPIndentWidth` to work that way too, it 
> would technically be a breaking change because users may have *explicitly* 
> specified `-1` in their YAML.

You need an explicit entry, because you need to be able to write the empty 
optional on `--dump-config`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

jp4a50 wrote:
> HazardyKnusperkeks wrote:
> > owenpan wrote:
> > > jp4a50 wrote:
> > > > owenpan wrote:
> > > > > owenpan wrote:
> > > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and 
> > > > > > somewhat confusing IMO.
> > > > > Please disregard my comment above.
> > > > Just to make sure we are on the same page, does this mean that you are 
> > > > happy with the approach of using `-1` as a default value to indicate 
> > > > that `ContinuationIndentWidth` should be used?
> > > > 
> > > > I did initially consider using `std::optional` and using 
> > > > empty optional to indicate that `ContinuationIndentWidth` should be 
> > > > used but I saw that `PPIndentWidth` was using `-1` to default to using 
> > > > `IndentWidth` so I followed that precedent.
> > > Yep! I would prefer the `optional`, but as you pointed out, we already 
> > > got `PPIndentWidth`using `-1`.
> > From the C++ side I totally agree. One could use `value_or()`, which would 
> > make the code much more readable.
> > And just because `PPIndentWidth` is using -1 is no reason to repeat that, 
> > we could just as easily change `PPIndentWidht` to an optional.
> > 
> > But how would it look in yaml?
> In YAML we wouldn't need to support empty optional being *explicitly* 
> specified - it would just be the default.
> 
> So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> `std::optional` to `4` but if `DesignatedInitializerIndentWidth` 
> was omitted from the YAML then the optional would simply not be set during 
> parsing.
> 
> Of course, if we were to change `PPIndentWidth` to work that way too, it 
> would technically be a breaking change because users may have *explicitly* 
> specified `-1` in their YAML.
> And just because `PPIndentWidth` is using -1 is no reason to repeat that, we 
> could just as easily change `PPIndentWidht` to an optional.

We would have to deal with backward compatibility to avoid regressions though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

HazardyKnusperkeks wrote:
> owenpan wrote:
> > jp4a50 wrote:
> > > owenpan wrote:
> > > > owenpan wrote:
> > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and 
> > > > > somewhat confusing IMO.
> > > > Please disregard my comment above.
> > > Just to make sure we are on the same page, does this mean that you are 
> > > happy with the approach of using `-1` as a default value to indicate that 
> > > `ContinuationIndentWidth` should be used?
> > > 
> > > I did initially consider using `std::optional` and using empty 
> > > optional to indicate that `ContinuationIndentWidth` should be used but I 
> > > saw that `PPIndentWidth` was using `-1` to default to using `IndentWidth` 
> > > so I followed that precedent.
> > Yep! I would prefer the `optional`, but as you pointed out, we already got 
> > `PPIndentWidth`using `-1`.
> From the C++ side I totally agree. One could use `value_or()`, which would 
> make the code much more readable.
> And just because `PPIndentWidth` is using -1 is no reason to repeat that, we 
> could just as easily change `PPIndentWidht` to an optional.
> 
> But how would it look in yaml?
In YAML we wouldn't need to support empty optional being *explicitly* specified 
- it would just be the default.

So specifying `DesignatedInitializerIndentWidth: 4` would set the 
`std::optional` to `4` but if `DesignatedInitializerIndentWidth` was 
omitted from the YAML then the optional would simply not be set during parsing.

Of course, if we were to change `PPIndentWidth` to work that way too, it would 
technically be a breaking change because users may have *explicitly* specified 
`-1` in their YAML.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

owenpan wrote:
> jp4a50 wrote:
> > owenpan wrote:
> > > owenpan wrote:
> > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and somewhat 
> > > > confusing IMO.
> > > Please disregard my comment above.
> > Just to make sure we are on the same page, does this mean that you are 
> > happy with the approach of using `-1` as a default value to indicate that 
> > `ContinuationIndentWidth` should be used?
> > 
> > I did initially consider using `std::optional` and using empty 
> > optional to indicate that `ContinuationIndentWidth` should be used but I 
> > saw that `PPIndentWidth` was using `-1` to default to using `IndentWidth` 
> > so I followed that precedent.
> Yep! I would prefer the `optional`, but as you pointed out, we already got 
> `PPIndentWidth`using `-1`.
From the C++ side I totally agree. One could use `value_or()`, which would make 
the code much more readable.
And just because `PPIndentWidth` is using -1 is no reason to repeat that, we 
could just as easily change `PPIndentWidht` to an optional.

But how would it look in yaml?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

jp4a50 wrote:
> owenpan wrote:
> > owenpan wrote:
> > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and somewhat 
> > > confusing IMO.
> > Please disregard my comment above.
> Just to make sure we are on the same page, does this mean that you are happy 
> with the approach of using `-1` as a default value to indicate that 
> `ContinuationIndentWidth` should be used?
> 
> I did initially consider using `std::optional` and using empty 
> optional to indicate that `ContinuationIndentWidth` should be used but I saw 
> that `PPIndentWidth` was using `-1` to default to using `IndentWidth` so I 
> followed that precedent.
Yep! I would prefer the `optional`, but as you pointed out, we already got 
`PPIndentWidth`using `-1`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked an inline comment as done.
jp4a50 added a comment.

All actionable comments have been addressed.




Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

owenpan wrote:
> owenpan wrote:
> > Using -1 to mean `ContinuationIndentWidth` is unnecessary and somewhat 
> > confusing IMO.
> Please disregard my comment above.
Just to make sure we are on the same page, does this mean that you are happy 
with the approach of using `-1` as a default value to indicate that 
`ContinuationIndentWidth` should be used?

I did initially consider using `std::optional` and using empty 
optional to indicate that `ContinuationIndentWidth` should be used but I saw 
that `PPIndentWidth` was using `-1` to default to using `IndentWidth` so I 
followed that precedent.



Comment at: clang/unittests/Format/FormatTest.cpp:4828
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);

MyDeveloperDay wrote:
> jp4a50 wrote:
> > MyDeveloperDay wrote:
> > > Can we fix the brace positioning too
> > What would you expect the brace positioning to be here? I noticed that, 
> > with this configuration at least, the closing brace goes at the end of the 
> > same line (as the last initializer) when there is no trailing comma but 
> > goes on a new line if there is a trailing comma.
> part of me would like an option to not have to supply the trailing comma, 
I agree that it's slightly odd behaviour but I think it's pretty orthogonal to 
this change and would deserve it's own diff.

I think it's also pretty debatable what the behaviour *should* be. Should the 
`AlignAfterOpenBracket` option dictate the formatting of braces like this? 
Oddly enough, specifying `AlwaysBreak` (as I have done in this test) does 
indeed result in breaking after the `{` but changing it to `BlockIndent` 
doesn't put the closing `}` on a newline. Should we instead add a new 
`AlignAfterOpenBrace` analogue of `AlignAfterOpenBracket`?

Would you like me to to raise a separate issue to track this (if it doesn't 
already exist)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 508740.
jp4a50 added a comment.

Address minor review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4820,6 +4820,34 @@
"[3] = cc,\n"
"[4] = dd,\n"
"[5] = ee};");
+
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
+  Style.DesignatedInitializerIndentWidth = 2;
+  verifyFormat("auto s = SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3};\n",
+   Style);
+  verifyFormat("auto s = someFunctionCall(\n"
+   ", ,\n"
+   "SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3});\n",
+   Style);
+  Style.ContinuationIndentWidth = 8;
+  Style.DesignatedInitializerIndentWidth = -1; // Use ContinuationIndentWidth.
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
 }
 
 TEST_F(FormatTest, NestedStaticInitializers) {
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -245,6 +245,10 @@
   SpacesBeforeTrailingComments, 1234u);
   CHECK_PARSE("IndentWidth: 32", IndentWidth, 32u);
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: 34",
+  DesignatedInitializerIndentWidth, 34);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: -1",
+  DesignatedInitializerIndentWidth, -1);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
   Style.QualifierAlignment = FormatStyle::QAS_Right;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -902,6 +902,8 @@
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth",
+   Style.DesignatedInitializerIndentWidth);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineAfterAccessModifier",
Style.EmptyLineAfterAccessModifier);
@@ -1367,6 +1369,7 @@
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = -1;
   LLVMStyle.DisableFormat = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1656,13 +1656,20 @@
 CurrentState.NestedBlockIndent);
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {
   NewIndent = Style.IndentWidth +
   std::min(State.Column, CurrentState.NestedBlockIndent);
+} else if (NextNoComment &&
+   NextNoComment->is(TT_DesignatedInitializerPeriod)) {
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? 

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/include/clang/Format/Format.h:2025
+  /// on a new line. When set to -1 (default), ``ContinuationIndentWidth`` is
+  /// used. \code
+  ///   AlignAfterOpenBracket: AlwaysBreak





Comment at: clang/include/clang/Format/Format.h:2037
+  /// \endcode
+  int DesignatedInitializerIndentWidth;
+

owenpan wrote:
> owenpan wrote:
> > 
> Please disregard my comment above.
Please add `\version 17`.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1659
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {

owenpan wrote:
> Can you also move it into the `else if` below to reduce the scope of the 
> pointer? Please note the typo `NextNoComment`.
Please ignore my comment above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/include/clang/Format/Format.h:2037
+  /// \endcode
+  int DesignatedInitializerIndentWidth;
+

owenpan wrote:
> 
Please disregard my comment above.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

owenpan wrote:
> Using -1 to mean `ContinuationIndentWidth` is unnecessary and somewhat 
> confusing IMO.
Please disregard my comment above.



Comment at: clang/lib/Format/Format.cpp:1372
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = -1;
   LLVMStyle.DisableFormat = false;

owenpan wrote:
> 
Please disregard my comment above.



Comment at: clang/unittests/Format/ConfigParseTest.cpp:250-251
+  DesignatedInitializerIndentWidth, 34);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: -1",
+  DesignatedInitializerIndentWidth, -1);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");

owenpan wrote:
> Delete.
Please disregard my comment above.



Comment at: clang/unittests/Format/FormatTest.cpp:4845
+  Style.ContinuationIndentWidth = 8;
+  Style.DesignatedInitializerIndentWidth = -1; // Use ContinuationIndentWidth.
+  verifyFormat("auto s = SomeStruct{\n"

owenpan wrote:
> Delete it and rearrange the tests so that the unspecified (default) 
> `DesignatedInitializerIndentWidth` is tested first.
Please disregard my comment above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D146101#4204653 , @jp4a50 wrote:

>>> I think it's also worth noting that the google style guide gives an example 
>>> of designated initializers indented at 2 spaces (whereas their 
>>> "continuation indent" for wrapped function parameters is 4).
>>
>> It's likely an error that the designated initializer example there shows 
>> 2-space indents as clang-format uses the 4-space continuation indent width:
>
> This is possible, but isn't it also possible that they would prefer 
> designated initializers to be indented at 2 spaces but don't have the option 
> with clang-format currently?

I really doubt it as the creators of clang-format and a number of 
reviewers/contributors/maintainers from the recent past were Google engineers, 
I believe.

> The only general information supplied in the google style guide about 
> indentation is as follows:
>
>> - Default indentation is 2 spaces.
>> - Wrapped parameters have a 4 space indent.
>
> If we are taking a strict definition of parameters, the above suggests to me 
> that designated initializers would fall under the default indentation of 2 
> spaces.

IMO it's clear that in Google style IndentWidth = 2 and ContinuationIndentWidth 
= 4. I think in general IndentWidth is for block indent and 
ContinuationIndentWidth is for everything else.




Comment at: clang/include/clang/Format/Format.h:2037
+  /// \endcode
+  int DesignatedInitializerIndentWidth;
+





Comment at: clang/lib/Format/ContinuationIndenter.cpp:1659
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {

Can you also move it into the `else if` below to reduce the scope of the 
pointer? Please note the typo `NextNoComment`.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

Using -1 to mean `ContinuationIndentWidth` is unnecessary and somewhat 
confusing IMO.



Comment at: clang/lib/Format/Format.cpp:1372
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = -1;
   LLVMStyle.DisableFormat = false;





Comment at: clang/unittests/Format/ConfigParseTest.cpp:250-251
+  DesignatedInitializerIndentWidth, 34);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: -1",
+  DesignatedInitializerIndentWidth, -1);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");

Delete.



Comment at: clang/unittests/Format/FormatTest.cpp:4845
+  Style.ContinuationIndentWidth = 8;
+  Style.DesignatedInitializerIndentWidth = -1; // Use ContinuationIndentWidth.
+  verifyFormat("auto s = SomeStruct{\n"

Delete it and rearrange the tests so that the unspecified (default) 
`DesignatedInitializerIndentWidth` is tested first.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D146101#4208962 , @MyDeveloperDay 
wrote:

> What I'd really like to see, is... in the event that 
> `ContinuationIndentWidth` is set and I do NOTset 
> `DesignatedInitializerIndentWidth`   , then 
> `DesignatedInitializerIndentWidth`  would inherit from that (not the default 
> as you have here),  if I set ContinuationIndentWidthto 3 
> DesignatedInitializerIndentWidth should be 3

Agree that this makes the most sense and, luckily, that is exactly the 
behaviour I implemented in my most recent diff!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

What I'd really like to see, is... in the event that `IndentWidth` is set and I 
do NOTset `DesignatedInitializerIndentWidth`   , then 
`DesignatedInitializerIndentWidth`  would inherit from that (not the default as 
you have here),  if I set IndentWidth to 3 DesignatedInitializerIndentWidth 
should be 3

Otherwise if I set `DesignatedInitializerIndentWidth` explicitly then it can be 
what is set,

i.e. (for base LLVM style)

  IndentWidth is not set = (default) 4
  DesignatedInitializerIndentWidth = not set  (implicitly 4)
  
  IndentWidth is set = 3
  DesignatedInitializerIndentWidth = not set  (implicitly 3)
  
  IndentWidth is not set = (default) 4
  DesignatedInitializerIndentWidth = is set to 2  (explicitly 2)
  
  IndentWidth is set = 3
  DesignatedInitializerIndentWidth = is set to 4 (explicitly 4)




Comment at: clang/unittests/Format/FormatTest.cpp:4828
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);

jp4a50 wrote:
> MyDeveloperDay wrote:
> > Can we fix the brace positioning too
> What would you expect the brace positioning to be here? I noticed that, with 
> this configuration at least, the closing brace goes at the end of the same 
> line (as the last initializer) when there is no trailing comma but goes on a 
> new line if there is a trailing comma.
part of me would like an option to not have to supply the trailing comma, 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

> The style guide doesn't mention indenting designated initializers with 2 
> spaces?

That is a fair point. I will get it updated because the author and maintainer 
of that style guide is the one requesting this change since, in practice, 
codebases following this style guide indent designated initializers with 2 
spaces.

>> I think it's also worth noting that the google style guide gives an example 
>> of designated initializers indented at 2 spaces (whereas their "continuation 
>> indent" for wrapped function parameters is 4).
>
> It's likely an error that the designated initializer example there shows 
> 2-space indents as clang-format uses the 4-space continuation indent width:

This is possible, but isn't it also possible that they would prefer designated 
initializers to be indented at 2 spaces but don't have the option with 
clang-format currently?

The only general information supplied in the google style guide about 
indentation is as follows:

> - Default indentation is 2 spaces.
> - Wrapped parameters have a 4 space indent.

If we are taking a strict definition of parameters, the above suggests to me 
that designated initializers would fall under the default indentation of 2 
spaces.

As mentioned on my other diff, I'm away until next Monday now so won't be able 
to get back to further comments til then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D146101#4201842 , @jp4a50 wrote:

> I understand the added complexity and maintenance burden of a new option but 
> we do meet the 3 criteria listed in your link.
>
> - it is part of the KJ style guide which is used by the capn proto 
>  project which has over 100 
> maintainers
> - the style guide is publicly accessible here 
> 

The style guide doesn't mention indenting designated initializers with 2 spaces?

> - I'm willing to contribute and maintain patches :)
>
> I think it's also worth noting that the google style guide 
>  
> gives an example of designated initializers indented at 2 spaces (whereas 
> their "continuation indent" for wrapped function parameters is 4).

It's likely an error that the designated initializer example there shows 
2-space indents as clang-format uses the 4-space continuation indent width:

  clang-format -style=Google
  Point p = {
  .x = 1.0, .y = 2.0,
  // z will be 0.0
  };
  Point p = {
  .x = 1.0, .y = 2.0,
  // z will be 0.0
  };


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D146101#4195293 , @owenpan wrote:

> Please see 
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.
>  Is there a way to fix the issue without adding a new option?

@owenpan We could simply change the indentation level of designated 
initializers for everyone to match `IndentWidth` instead of 
`ContinuationIndentWidth` but I suspect that other users might be unhappy if we 
made that breaking change? I guess we could also consider making this behaviour 
only kick in when a specific set of *other* options are specified (e.g. 
`AlwaysBreak`) so that it applies to our clang-format style and not *all* 
others but that just feels like a hack.

I understand the added complexity and maintenance burden of a new option but we 
do meet the 3 criteria listed in your link.

- it is part of the KJ style guide which is used by the capn proto 
 project which has over 100 maintainers
- the style guide is publicly accessible here 

- I'm willing to contribute and maintain patches :)

I think it's also worth noting that the google style guide 
 
gives an example of designated initializers indented at 2 spaces (whereas their 
"continuation indent" for wrapped function parameters is 4).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 2 inline comments as done.
jp4a50 added inline comments.



Comment at: clang/lib/Format/Format.cpp:1372
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;

jp4a50 wrote:
> MyDeveloperDay wrote:
> > so lets say someone is using an IndentWidth of 2 and now you introduce this 
> > as being 4 here as the default
> > 
> > Without them changing anything, all their DesignatedIntializer code will 
> > get refactored to a IndentWidth of 4 rather than the 2 it was previously
> > 
> > This is where we get called out for "changing the defaults", which really 
> > we are not we are just reclassifying how it behaves.
> > 
> > What we normally say here is can you point us to a public style that has an 
> > independent DesignatedIntializerIndentWidth which is independent from the 
> > levels of IndentWidth everywhere else.
> > 
> > Whilst I can see more knobs feels good, this will change code and we'll 
> > have to manage that expectation.
> > 
> > 
> > 
> Yep, so as per my comment in `clang/docs/ClangFormatStyleOptions.rst` I think 
> I am changing my mind about this anyway.
> 
> My motivation for making this change is to support the [[ 
> https://github.com/capnproto/capnproto/blob/master/kjdoc/style-guide.md | KJ 
> style guide ]] which is quoted in the [[ 
> https://github.com/llvm/llvm-project/issues/51070 | github issue ]] - I work 
> on a team that uses the KJ style guide.
> 
> The KJ style guide wants a designated initializer indent width of 2 along 
> with a "normal" indent width of 2, so there is no explicit need for us to 
> have those two values be different.
> 
> When originally making these changes, I did think that having "more knobs" 
> was a good idea, but I agree that this could lead to annoying behaviour for 
> some users and they would probably expect the designated initializer indent 
> to match either the normal indent or the continuation indent.
> 
> How about I change the option to an integer and, when it's -1 (the default), 
> the designated initializer indent matches the continuation indent, but if it 
> is set to a value >= 0 then that number of columns is used instead? 
> 
I've reimplemented the option as a signed integer as per my suggestion above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 505540.
jp4a50 added a comment.

Change DesignatedInitializerIndentWidth to a signed integer which defaults to 
ContinuationIndentWidth when set to -1.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4820,6 +4820,34 @@
"[3] = cc,\n"
"[4] = dd,\n"
"[5] = ee};");
+
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
+  Style.DesignatedInitializerIndentWidth = 2;
+  verifyFormat("auto s = SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3};\n",
+   Style);
+  verifyFormat("auto s = someFunctionCall(\n"
+   ", ,\n"
+   "SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3});\n",
+   Style);
+  Style.ContinuationIndentWidth = 8;
+  Style.DesignatedInitializerIndentWidth = -1; // Use ContinuationIndentWidth.
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
 }
 
 TEST_F(FormatTest, NestedStaticInitializers) {
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -245,6 +245,10 @@
   SpacesBeforeTrailingComments, 1234u);
   CHECK_PARSE("IndentWidth: 32", IndentWidth, 32u);
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: 34",
+  DesignatedInitializerIndentWidth, 34);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: -1",
+  DesignatedInitializerIndentWidth, -1);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
   Style.QualifierAlignment = FormatStyle::QAS_Right;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -902,6 +902,8 @@
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth",
+   Style.DesignatedInitializerIndentWidth);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineAfterAccessModifier",
Style.EmptyLineAfterAccessModifier);
@@ -1367,6 +1369,7 @@
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = -1;
   LLVMStyle.DisableFormat = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1656,13 +1656,20 @@
 CurrentState.NestedBlockIndent);
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {
   NewIndent = Style.IndentWidth +
   std::min(State.Column, CurrentState.NestedBlockIndent);
+} else if (NextNoComment &&
+   NextNoComment->is(TT_DesignatedInitializerPeriod)) {
+  const auto DesignatedInitializerIndentWidth =
+  

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/Format.cpp:1372
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;

MyDeveloperDay wrote:
> so lets say someone is using an IndentWidth of 2 and now you introduce this 
> as being 4 here as the default
> 
> Without them changing anything, all their DesignatedIntializer code will get 
> refactored to a IndentWidth of 4 rather than the 2 it was previously
> 
> This is where we get called out for "changing the defaults", which really we 
> are not we are just reclassifying how it behaves.
> 
> What we normally say here is can you point us to a public style that has an 
> independent DesignatedIntializerIndentWidth which is independent from the 
> levels of IndentWidth everywhere else.
> 
> Whilst I can see more knobs feels good, this will change code and we'll have 
> to manage that expectation.
> 
> 
> 
Yep, so as per my comment in `clang/docs/ClangFormatStyleOptions.rst` I think I 
am changing my mind about this anyway.

My motivation for making this change is to support the [[ 
https://github.com/capnproto/capnproto/blob/master/kjdoc/style-guide.md | KJ 
style guide ]] which is quoted in the [[ 
https://github.com/llvm/llvm-project/issues/51070 | github issue ]] - I work on 
a team that uses the KJ style guide.

The KJ style guide wants a designated initializer indent width of 2 along with 
a "normal" indent width of 2, so there is no explicit need for us to have those 
two values be different.

When originally making these changes, I did think that having "more knobs" was 
a good idea, but I agree that this could lead to annoying behaviour for some 
users and they would probably expect the designated initializer indent to match 
either the normal indent or the continuation indent.

How about I change the option to an integer and, when it's -1 (the default), 
the designated initializer indent matches the continuation indent, but if it is 
set to a value >= 0 then that number of columns is used instead? 




Comment at: clang/unittests/Format/FormatTest.cpp:4828
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);

MyDeveloperDay wrote:
> Can we fix the brace positioning too
What would you expect the brace positioning to be here? I noticed that, with 
this configuration at least, the closing brace goes at the end of the same line 
(as the last initializer) when there is no trailing comma but goes on a new 
line if there is a trailing comma.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:1372
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;

so lets say someone is using an IndentWidth of 2 and now you introduce this as 
being 4 here as the default

Without them changing anything, all their DesignatedIntializer code will get 
refactored to a IndentWidth of 4 rather than the 2 it was previously

This is where we get called out for "changing the defaults", which really we 
are not we are just reclassifying how it behaves.

What we normally say here is can you point us to a public style that has an 
independent DesignatedIntializerIndentWidth which is independent from the 
levels of IndentWidth everywhere else.

Whilst I can see more knobs feels good, this will change code and we'll have to 
manage that expectation.





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

Please see 
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.
 Is there a way to fix the issue without adding a new option?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Format/Format.h:2031
+  /// };
+  ///   }
+  unsigned DesignatedInitializerIndentWidth;

Endcode



Comment at: clang/lib/Format/Format.cpp:905
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth",
+   Style.DesignatedInitializerIndentWidth);

Needs a parse test



Comment at: clang/unittests/Format/FormatTest.cpp:4828
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);

Can we fix the brace positioning too


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2704
+
+**DesignatedInitializerIndentWidth** (``Unsigned``)
+  The number of columns to use to indent designated initializers that start on 
a new line.

Perhaps it would be better to make this a signed integer and default to `-1` 
which would cause `ContinuationIndentWidth` to be used (i.e. current behaviour)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 505302.
jp4a50 added a comment.

Apply clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4820,6 +4820,26 @@
"[3] = cc,\n"
"[4] = dd,\n"
"[5] = ee};");
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
+  Style.DesignatedInitializerIndentWidth = 2;
+  verifyFormat("auto s = SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3};\n",
+   Style);
+  verifyFormat("auto s = someFunctionCall(\n"
+   ", ,\n"
+   "SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3});\n",
+   Style);
 }
 
 TEST_F(FormatTest, NestedStaticInitializers) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -902,6 +902,8 @@
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth",
+   Style.DesignatedInitializerIndentWidth);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineAfterAccessModifier",
Style.EmptyLineAfterAccessModifier);
@@ -1367,6 +1369,7 @@
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1656,13 +1656,16 @@
 CurrentState.NestedBlockIndent);
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {
   NewIndent = Style.IndentWidth +
   std::min(State.Column, CurrentState.NestedBlockIndent);
+} else if (NextNoComment->is(TT_DesignatedInitializerPeriod)) {
+  NewIndent =
+  CurrentState.LastSpace + Style.DesignatedInitializerIndentWidth;
 } else {
   NewIndent = CurrentState.LastSpace + Style.ContinuationIndentWidth;
 }
-const FormatToken *NextNoComment = Current.getNextNonComment();
 bool EndsInComma = Current.MatchingParen &&
Current.MatchingParen->Previous &&
Current.MatchingParen->Previous->is(tok::comma);
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -2020,6 +2020,17 @@
   /// \version 3.7
   bool DerivePointerAlignment;
 
+  /// The number of columns to use to indent designated initializers that start
+  /// on a new line. \code
+  ///   void f() {
+  /// auto s = SomeStruct{
+  ///   .foo = "foo",
+  ///   .bar = "bar",
+  ///   .baz = "baz",
+  /// };
+  ///   }
+  unsigned DesignatedInitializerIndentWidth;
+
   /// Disables formatting completely.
   /// \version 3.7
   bool DisableFormat;
@@ -4236,6 +4247,8 @@
ContinuationIndentWidth == R.ContinuationIndentWidth &&
Cpp11BracedListStyle == R.Cpp11BracedListStyle &&
DerivePointerAlignment == R.DerivePointerAlignment &&
+   

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 created this revision.
Herald added a project: All.
jp4a50 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The option allows users to specify how many columns to use to indent
designated initializers that start on a new line.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4820,6 +4820,25 @@
"[3] = cc,\n"
"[4] = dd,\n"
"[5] = ee};");
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
+  Style.DesignatedInitializerIndentWidth = 2;
+  verifyFormat("auto s = SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3};\n",
+   Style);
+  verifyFormat("auto s = someFunctionCall(\n"
+   ", ,\n"
+   "SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3});\n", Style);
 }
 
 TEST_F(FormatTest, NestedStaticInitializers) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -902,6 +902,7 @@
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth", Style.DesignatedInitializerIndentWidth);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineAfterAccessModifier",
Style.EmptyLineAfterAccessModifier);
@@ -1367,6 +1368,7 @@
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1656,13 +1656,15 @@
 CurrentState.NestedBlockIndent);
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {
   NewIndent = Style.IndentWidth +
   std::min(State.Column, CurrentState.NestedBlockIndent);
+} else if (NextNoComment->is(TT_DesignatedInitializerPeriod)) {
+  NewIndent = CurrentState.LastSpace + Style.DesignatedInitializerIndentWidth;
 } else {
   NewIndent = CurrentState.LastSpace + Style.ContinuationIndentWidth;
 }
-const FormatToken *NextNoComment = Current.getNextNonComment();
 bool EndsInComma = Current.MatchingParen &&
Current.MatchingParen->Previous &&
Current.MatchingParen->Previous->is(tok::comma);
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -2020,6 +2020,17 @@
   /// \version 3.7
   bool DerivePointerAlignment;
 
+  /// The number of columns to use to indent designated initializers that start on a new line.
+  /// \code
+  ///   void f() {
+  /// auto s = SomeStruct{
+  ///   .foo = "foo",
+  ///   .bar = "bar",
+  ///   .baz = "baz",
+  /// };
+  ///   }
+  unsigned DesignatedInitializerIndentWidth;
+
   /// Disables formatting completely.
   /// \version 3.7
   bool DisableFormat;
@@ -4236,6 +4247,7 @@
ContinuationIndentWidth == R.ContinuationIndentWidth &&
Cpp11BracedListStyle == R.Cpp11BracedListStyle &&