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<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! 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
