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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
