MyDeveloperDay marked 3 inline comments as done. MyDeveloperDay added a comment.
In D69764#2058801 <https://reviews.llvm.org/D69764#2058801>, @curdeius wrote: > One last thought, how about making the config a bit more future-proof and > instead of `ConstPlacement` accept what was discussed some time ago: > > QualifierStyle: > - const: Right > > > and restrict it to `const` for the moment? > Maybe renaming to `QualifierPlacement` or something more appropriate. I'm already feeling there needs to be more fine grained control here in the config... (even if that is a list of MACRO types to ignore) or IgnoreCapitializedIdentifiers, or WarnButDontFix or as we talked about before handling more than just the placement of const, I feel you are correct I'll end up polluting the toplevel config namespace unless I switch to some form of structure in the YAML like we do with BraceWrapping. ================ Comment at: clang/lib/Format/EastWestConstFixer.cpp:157-158 + IsCVQualifierOrType(Tok->Next->Next)) { + // The unsigned/signed case `const unsigned int` -> `unsigned int + // const` + swapFirstThreeTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next->Next, ---------------- MyDeveloperDay wrote: > aaron.ballman wrote: > > What about something like `const unsigned long long` or the > > less-common-but-equally-valid `long const long unsigned`? Or multiple > > qualifiers like `unsigned volatile long const long * restrict` > I would like to cover as many cases as possible, but a small part of me > thinks that at least initially if we skip a case or two like this I'd be fine. > > But I'll take a look and see what I think we could add easily in terms of > multiple simple types in a row. @aaron.ballman if you saw `long const long unsigned` what would you expect it to become in both east and west const cases? my assumption is: - east : `long long unsigned const` - west: `const long long unsigned` Or something else? same for `unsigned volatile long const long * restrict` I assume: - east : `unsigned volatile long long const * restrict` - west: ` const unsigned volatile long long* restrict` Its it would help if I understood the typical expectation in these situations? ================ Comment at: clang/lib/Format/EastWestConstFixer.cpp:195 + FormatToken *Tok) { + // We only need to think about streams that begin with const. + if (!Tok->is(tok::kw_const)) { ---------------- curdeius wrote: > Why? What about `unsigned const int`? @curdeius would you help me understand your expectation here? - east: `unsigned int const` - west: `const unsigned int` ? ================ Comment at: clang/lib/Format/EastWestConstFixer.cpp:293 + Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) { + swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next, Tok->Next->Next->Next, + /*West=*/true); ---------------- rsmith wrote: > There can be more than four type-specifiers / cv-qualifiers in a row. Eg: > `unsigned long long int volatile const` -> `const volatile unsigned long long > int`. you have the volatile moving too? if you had the choice would it become: - `const unsigned long long int volatile` - `const volatile unsigned long long int` - `volatile const unsigned long long int` Any reason why? or is that personal taste? what would be the ideal? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
