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

Wed Apr 12 13:07:28 PDT 2023

owenpan accepted this revision.
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;
----------------
HazardyKnusperkeks wrote:
> jp4a50 wrote:
> > owenpan wrote:
> > > 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<unsigned>` 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<unsigned>` 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<unsigned>` 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<unsigned>` 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 with experience with YAML, but with no experience with LLVM's YAML stuff, I'd suggest making it `null` (explicitly), but a) i don't know if that's supported and b) i'm not sure if it's semantically any clearer than just `-1`
> > > > > I'd do the right think with `DesignatedInitializerIndentWidth` which I guess is to use the `std::optional` that @owenpan suggests and don't worry about `PPIndentWidth` for now,  
> > > > > 
> > > > > if anything if it works I'd prefer to understand if we can turn `PPIndentWidth`  into a `std::optional` later (in a seperate review) and just catch the -1 case so at least the code is nicer, but that is a different task
> > > > > 
> > > > > 
> > > > > > > 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<unsigned>` 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 @MyDeveloperDay said, ignore `PPIndentWidth`, that will be dealt with on a different occasion. Use the optional, it is the right thing (TM) to do.
> > > > 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.
> > > > I'd do the right think with `DesignatedInitializerIndentWidth` which I guess is to use the `std::optional` that @owenpan suggests and don't worry about `PPIndentWidth` for now
> > > 
> > > +1.
> > > > > > 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<unsigned>` 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 @MyDeveloperDay said, ignore `PPIndentWidth`, that will be dealt with on a different occasion. Use the optional, it is the right thing (TM) to do.
> > > 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.
> > 
> > @HazardyKnusperkeks @owenpan, before potentially committing this change, I just wanted to draw your attention again to this comment to confirm that you are happy with the current implementation which doesn't explicitly print `null` or similar for a default value of `DesignatedInitializerIndentWidth` when dumping the config. I'm assuming that's OK since @HazardyKnusperkeks suggested that we don't bother if it involves messing around with the yaml code (which it would).
> > @HazardyKnusperkeks @owenpan, before potentially committing this change, I just wanted to draw your attention again to this comment to confirm that you are happy with the current implementation which doesn't explicitly print `null` or similar for a default value of `DesignatedInitializerIndentWidth` when dumping the config. I'm assuming that's OK since @HazardyKnusperkeks suggested that we don't bother if it involves messing around with the yaml code (which it would).
> 
> Sure, everything is fine.
Looks good.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146101