[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

simon.giesecke added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:3233 + + ``QualifierAlignment`` COULD lead to incorrect code generation. + MyDeveloperDay wrote: > HazardyKnusperkeks wrote: > > simon.giesecke wrote: > > > This is pretty

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay marked an inline comment as done. MyDeveloperDay added a comment. I'm not a wordsmith but how about D110801: [clang-format] [docs] [NFC] improve clarity in the QualifierAlignment warning Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay marked 2 inline comments as done. MyDeveloperDay added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:3233 + + ``QualifierAlignment`` COULD lead to incorrect code generation. + HazardyKnusperkeks wrote: > simon.giesecke

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

HazardyKnusperkeks added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:3233 + + ``QualifierAlignment`` COULD lead to incorrect code generation. + simon.giesecke wrote: > This is pretty unclear, for a number of reasons: > * First, this

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

simon.giesecke added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:3233 + + ``QualifierAlignment`` COULD lead to incorrect code generation. + This is pretty unclear, for a number of reasons: * First, this probably only refers to a setting

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added a comment. In D69764#3019409 , @nemanjai wrote: > This broke buildbots that have -Werror specified. I pushed in a fix in > https://reviews.llvm.org/rG76d845cb169f048cb6f2176c3e7a6534dc5af097. > Also, please consider formatting your

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

nemanjai added a comment. This broke buildbots that have -Werror specified. I pushed in a fix in https://reviews.llvm.org/rG76d845cb169f048cb6f2176c3e7a6534dc5af097. Also, please consider formatting your commit messages to prevent wrapping. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa44ab1702539: [clang-format] Add Left/Right Const fixer capability (authored by MyDeveloperDay). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added a comment. If there are no other objections I'm going to land this shortly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

HazardyKnusperkeks accepted this revision. HazardyKnusperkeks added a comment. This revision is now accepted and ready to land. Let's give it a first shot. When it has landed maybe I find the time to look into the attributes. ;) Comment at:

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:449 +return false; + if (Tok->is(TT_TypenameMacro)) +return false; HazardyKnusperkeks wrote: > I think this is still right, because it is a type (according to

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 374455. MyDeveloperDay marked 3 inline comments as done. MyDeveloperDay added a comment. Fix clang-format and clang-tidy issues CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files:

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

HazardyKnusperkeks added inline comments. Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:449 +return false; + if (Tok->is(TT_TypenameMacro)) +return false; I think this is still right, because it is a type (according to the configuration).

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 374298. MyDeveloperDay added a comment. Use better vector initialization CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files: clang/docs/ClangFormatStyleOptions.rst clang/docs/ReleaseNotes.rst

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 374294. MyDeveloperDay marked 3 inline comments as done. MyDeveloperDay added a comment. Remove use of Typenamemacros (we'll solve this a different way later) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

HazardyKnusperkeks added inline comments. Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:57 + + std::string NewText = " " + Qualifier + " "; + NewText += Next->TokenText; MyDeveloperDay wrote: > HazardyKnusperkeks wrote: > > Does not need to be

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 373954. MyDeveloperDay added a comment. Its no longer essential to compare the fixes against the original code, as this was needed because of an artifact with adding double spaces and was resolved with checking for if the previous token already ended

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 373817. MyDeveloperDay added a comment. If we don't specify types in the QualifierOrder (say static or inline as in default Left/Right) then don't push const through them just because they can be a type of qualifier we support. i.e. only push

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:28 + +static void replaceToken(const SourceManager , + tooling::Replacements , HazardyKnusperkeks wrote: > Out of curiosity, on what premise do

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 373802. MyDeveloperDay added a comment. Allow the use of "TypenameMacros" to allow for Capitialized types (be aware they should be non pointer types and don't have to be macros, so may need its own future "NonPtrTypes" setting) CHANGES SINCE LAST

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 373795. MyDeveloperDay marked 11 inline comments as done. MyDeveloperDay added a comment. Address review comments. - reorder functions to match header Fix an overlapping replacement issue when going from Right to Left (just jump past the token once

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

HazardyKnusperkeks added a comment. Really nice! No attributes in there, do you think it would be difficult to add them? We can definitely do that in another change to move this one forward. Comment at: clang/include/clang/Format/Format.h:1863-1864 + /// \warning + ///

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 373681. MyDeveloperDay added a comment. Remove debug code Tidy a few comments Remove Qualifier Order defaults (must be specified for Custom) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files:

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 373657. MyDeveloperDay added a comment. Missed dump_format_style.py update CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files: clang/docs/ClangFormatStyleOptions.rst

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 373656. MyDeveloperDay marked an inline comment as done. MyDeveloperDay added a comment. - Resolve the final issue I had regarding overlapping replacements (was adding double spaces between keywords), - Add ability to add a warning into the

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay marked 3 inline comments as done. MyDeveloperDay added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:2101 +**CVQualifierAlignment** (``CVQualifierAlignmentStyle``) + Different ways to arrange const/volatile qualifiers.

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 372430. MyDeveloperDay added a comment. Allow more QualifierFixer functions to be directly tested, remove some older commented code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files:

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

aaron.ballman added a comment. In D69764#2997910 , @MyDeveloperDay wrote: > I think the relevance of Left/Right East/West as a setting is less > important, as its more about an ordering, allowing some tokens to go to the > Left and some to the Right

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 372292. MyDeveloperDay added a comment. QualifierAlignmentFixer need to process all the Left/Right passes internally before return the fixes on the original code. So now QualifierAlignmentFixer has its own inner set of passes which will be processed,

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added a comment. I think the relevance of Left/Right East/West as a setting is less important, as its more about an ordering, allowing some tokens to go to the Left and some to the Right of the "type" QualifierAlignment: Custom QualifierOrder: [ inline, static, type, const,

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

aaron.ballman added a comment. In D69764#2996365 , @steveire wrote: > FYI - there's nothing "anti-inclusive" about East/West. While I'm certain that there can be confusion around why terms are or are not troublesome to some people and there needs to be

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

curdeius added a comment. In D69764#2996365 , @steveire wrote: > FYI - there's nothing "anti-inclusive" about East/West. Thank you for your comment. Personally I don't think that the question of not using East/West is whether these terms are inclusive

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

steveire added a comment. FYI - there's nothing "anti-inclusive" about East/West. In Qt I proposed a change which added properties to an object. Those properties in my change were named "Left"/"Right". The reviewer objected to those names because "scripts which are not Right-to-Left exist". So

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 365765. MyDeveloperDay added a comment. - Remove the "CV" from the options - Add support for ordering static/inline/const/volatile around the type, allowing for mixing both left and right alignment at the same time QualifierAlignment: Custom

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

aaron.ballman added a comment. In D69764#2938973 , @MyDeveloperDay wrote: > With the introduction of `CVQualifierOrder` the need for > `CVQualifierAlignment:` doesn't make so much sense > > CVQualifierAlignment: Left > CVQualifierOrder: [ "inline",

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added a subscriber: asb. MyDeveloperDay added a comment. > In D69764#2939037 , > @HazardyKnusperkeks wrote: > I would like to remove the CV, only QualifierOrder. Oh gosh, I agree here so much with both you and @curdeius , I keep miss

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

curdeius added a comment. In D69764#2939037 , @HazardyKnusperkeks wrote: > In D69764#2938973 , @MyDeveloperDay > wrote: > >> I'm leaning toward introducing into the `CVQualifierOrder` allowing >> for some

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

HazardyKnusperkeks added a comment. In D69764#2938973 , @MyDeveloperDay wrote: > With the introduction of `CVQualifierOrder` the need for > `CVQualifierAlignment:` doesn't make so much sense > > CVQualifierAlignment: Left > CVQualifierOrder: [

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added a comment. With the introduction of `CVQualifierOrder` the need for `CVQualifierAlignment:` doesn't make so much sense CVQualifierAlignment: Left CVQualifierOrder: [ "inline", "static", "volatile", "const" ] I'm leaning toward introducing into the `CVQualifierOrder`

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 365519. MyDeveloperDay added a comment. - Rename the ConstFixer to QualifierAligmentFixer (as now we handle more than just const) and in preparation for handling others CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 365502. MyDeveloperDay added a comment. - Add CVQualifierOrder configuration validation and unit tests CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files: clang/docs/ClangFormatStyleOptions.rst

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added a comment. > In D69764#2936827 , > @HazardyKnusperkeks wrote: > >> First off, I think it should be configured in a different way, to prepare >> the path for also formatting static, inline, etc. I'm wondering if I'm gravitating more

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added a comment. In D69764#2936827 , @HazardyKnusperkeks wrote: > First off, I think it should be configured in a different way, to prepare the > path for also formatting static, inline, etc. To be honest I think if we wanted to do that

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 365472. MyDeveloperDay added a comment. - elide the braces - use references CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files: clang/docs/ClangFormatStyleOptions.rst

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

HazardyKnusperkeks added a comment. First off, I think it should be configured in a different way, to prepare the path for also formatting static, inline, etc. If this is kept there should be tests on what happens if there is const or volatile more than once in the string list, and when there

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 365456. MyDeveloperDay marked 2 inline comments as done. MyDeveloperDay added a comment. - Add support for both const and volatile alignment - change `ConstPlacement` to `CVQualifierAlignment` - add `CVQualifierOrder` to allow control over `const

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 365276. MyDeveloperDay retitled this revision from "[clang-format] Add East/West Const fixer capability" to "[clang-format] Add Left/Right Const fixer capability". MyDeveloperDay added a comment. Remove East/West terminology for inclusivity CHANGES

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

aaron.ballman added a comment. In D69764#2050226 , @steveire wrote: > I like the approach of using clang-format to implement this. It's much faster > than a `clang-tidy` approach. > > The broader C++ community has already chosen `East`/`West` and it has

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

steveire added a comment. In D69764#2050538 , @MyDeveloperDay wrote: > In D69764#2050226 , @steveire wrote: > > > I like the approach of using clang-format to implement this. It's much > > faster than a

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay added a comment. In D69764#2050226 , @steveire wrote: > I like the approach of using clang-format to implement this. It's much faster > than a `clang-tidy` approach. > > The broader C++ community has already chosen `East`/`West` and it

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

steveire added a comment. I like the approach of using clang-format to implement this. It's much faster than a `clang-tidy` approach. The broader C++ community has already chosen `East`/`West` and it has momentum. If you choose `Left`/`Right` now, you will get pressure to add `East`/`West` in

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 262779. MyDeveloperDay added a comment. Refactor the analyse function to reduce the function size CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files: clang/docs/ClangFormatStyleOptions.rst

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 262772. MyDeveloperDay added a comment. remove macros from the unit tests CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files: clang/docs/ClangFormatStyleOptions.rst clang/docs/ReleaseNotes.rst

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

MyDeveloperDay updated this revision to Diff 262768. MyDeveloperDay added a comment. I'm returning to this revision which I'd left to think about, this update is really just a rebase and also to remove the dual configuation. For now I will still with just Left/Right const to avoid confusion.