[PATCH] D69764: [clang-format] Add East/West Const fixer capability

erichkeane resigned from this revision. erichkeane added a comment. This revision now requires review to proceed. Not sure if this will clear the 'needs revision', but an RFC has been sent. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. In D69764#2935269 , @erichkeane wrote: > In D69764#2935218 , @MyDeveloperDay > wrote: > >> In D69764#2934666 , @erichkeane >> wrote: >>

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

erichkeane added a comment. In D69764#2935218 , @MyDeveloperDay wrote: > In D69764#2934666 , @erichkeane > wrote: > >>> Someone internally pointed out the anti-inclusivity of the terminology, so >>> I figured

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. In D69764#2934666 , @erichkeane wrote: >> Someone internally pointed out the anti-inclusivity of the terminology, so I >> figured I'd bring it up. I apologise if I'm proliferating that, but could you educate me why its

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. In D69764#2935036 , @klimek wrote: > Happy to go the RFC route if @MyDeveloperDay wants to do that. I'm happy to do that (in fact I've written a draft), do people want to code review the RFC draft (as I could easily be

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

klimek added a comment. In D69764#2934686 , @aaron.ballman wrote: > In D69764#2934612 , @MyDeveloperDay > wrote: > >>> I was referring to @rsmith and @aaron.ballman (to clarify), both are >>> maintainers in

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2934612 , @MyDeveloperDay wrote: >> I was referring to @rsmith and @aaron.ballman (to clarify), both are >> maintainers in 'clang', the former of which is the 'superset' maintainer of >> this format project

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

erichkeane added a comment. In D69764#2934646 , @MyDeveloperDay wrote: >> I find it weird that we aren't handling ALL of the CV qualifiers. > > I will probably try and address this, I do have some ideas, but this will I > believe complicate the

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > I find it weird that we aren't handling ALL of the CV qualifiers. I will probably try and address this, I do have some ideas, but this will I believe complicate the implementation. For now I really want to understand if conceptually such a feature can be

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > I was referring to @rsmith and @aaron.ballman (to clarify), both are > maintainers in 'clang', the former of which is the 'superset' maintainer of > this format project based on its directory. While Aaron is a peer-maintainer > to this project, his

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

erichkeane added a comment. In D69764#2934560 , @MyDeveloperDay wrote: >> It _IS_ a democracy where we can find a fair consensus! And the mechanism >> with which to obtain said `fair consensus` is an RFC. > > Then I think in the interest of finding one

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > It _IS_ a democracy where we can find a fair consensus! And the mechanism > with which to obtain said `fair consensus` is an RFC. Then I think in the interest of finding one we should start with the RFC. CHANGES SINCE LAST ACTION

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

erichkeane added a comment. In D69764#2934535 , @MyDeveloperDay wrote: >> My 'requires changes' is that this needs an LLVM-project-level RFC to change >> the charter of one of its projects, doing so in a 15 month long patch, >> against the wishes of

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > My 'requires changes' is that this needs an LLVM-project-level RFC to change > the charter of one of its projects, doing so in a 15 month long patch, > against the wishes of TWO maintainers is a violation of the LLVM community > practice. I'm completely

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

erichkeane added a comment. In D69764#2934489 , @MyDeveloperDay wrote: > In D69764#2934378 , @erichkeane > wrote: > >> I've just been watching this from the sideline, but the cases where this >> breaks code are

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. In D69764#2934483 , @erichkeane wrote: > In D69764#2934473 , @MyDeveloperDay > wrote: > >> In D69764#2934378 , @erichkeane >> wrote: >> >>>

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. In D69764#2934378 , @erichkeane wrote: > I've just been watching this from the sideline, but the cases where this > breaks code are unacceptable for this tool, it is a complete direction change > for the tool, and making

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

erichkeane added a comment. In D69764#2934473 , @MyDeveloperDay wrote: > In D69764#2934378 , @erichkeane > wrote: > >> I've just been watching this from the sideline, but the cases where this >> breaks code are

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. In D69764#2934378 , @erichkeane wrote: > I've just been watching this from the sideline, but the cases where this > breaks code are unacceptable for this tool, it is a complete direction change > for the tool, and making

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

erichkeane requested changes to this revision. erichkeane added a comment. This revision now requires changes to proceed. I've just been watching this from the sideline, but the cases where this breaks code are unacceptable for this tool, it is a complete direction change for the tool, and

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

sammccall added a comment. For my part, I'm convinced and now +1 (or at least +0.5) on this being OK to include. In users' minds this is a formatting/style operation, and the UX of clang-format and its integrations is much better than clang-tidy. Implementation quality problems are a risk, but

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2934124 , @klimek wrote: > In D69764#2934032 , @MyDeveloperDay > wrote: > >> It was suggested to me that I write up and RFC on the matter, I'm not a >> massive fan of

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

klimek added a comment. In D69764#2934032 , @MyDeveloperDay wrote: > In D69764#2932929 , @steveire wrote: > >> > > @steveire, thanks for your comments, I also agree that a second tool > shouldn't be needed,

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added subscribers: djasper, klimek. MyDeveloperDay added a comment. In D69764#2932929 , @steveire wrote: > @steveire, thanks for your comments, I also agree that a second tool shouldn't be needed, especially as this functionality is off

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. Making a separate tool for this makes no sense. Especially as you are only proposing it to satisfy one (or are there more) vocal objector. The objections to this make no sense. If you don't want to use it, then don't enable it. That principle applies whether "the way

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. In D69764#2877618 , @atomgalaxy wrote: > It would probably be better to make the config option names for passes that > may mutate whitespace be prefixed with MaybeIncorrect than fork the tool. > Scary options are better

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

HazardyKnusperkeks added a comment. In D69764#2876916 , @MyDeveloperDay wrote: >> So yes, I'm in favour of landing this patch (though not exactly in the >> current form, I'd prefer more future-proof options for instance, not only >> handling const) > >

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

It would probably be better to make the config option names for passes that may mutate whitespace be prefixed with MaybeIncorrect than fork the tool. Scary options are better than forks. On Wed, Jul 14, 2021 at 6:42 PM MyDeveloperDay via Phabricator < revi...@reviews.llvm.org> wrote: >

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. I wanted to address your other points so we at least can be aligned as I respect your opinion. > I'm however even more wary of adding yet another tool that will be almost the > same as clang-format. Also agreed, but I see no other way forward. > It could work

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

+1 for not only handling "const". I've often tried getting the various bits that appertain to a declaration (static const volatile constexpr inline consteval) sorted in a consistent order - that makes them much more greppable. Different patch, I expect, though. On Wed, Jul 14, 2021 at 1:47 PM

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > So yes, I'm in favour of landing this patch (though not exactly in the > current form, I'd prefer more future-proof options for instance, not only > handling const) I am in agreement, but I don't want to not putting more effort into improving the current

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

curdeius added a comment. I've been trying to make my opinion on this patch for the last few weeks... I was pretty much opposed to introducing non-whitespace chances previously, but I'm reviewing my standpoint. As mentioned already, there are precedents (include sorting, namespace comments,

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

It's very difficult to use a compile_commands database if you can't actually check out all the code and a remote service builds it for you. On Tue, Jul 13, 2021 at 6:03 PM Aaron Ballman via Phabricator < revi...@reviews.llvm.org> wrote: > aaron.ballman added a comment. > > In D69764#2874404

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. Interesting reference point here https://blog.jetbrains.com/rscpp/2021/03/30/resharper-cpp-2021-1-syntax-style Kind of suggests that Resharper brings many of these "mutating" capabilities into the same tool that fixes your style. I'd be really interest to know

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. Because I don't want to move this review on other than to maintain it for future rebasing, I've created a new review for the concept of the new tool D105943: [clang-format++] Create a new variant of the clang-format tool to allow additional code mutating

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

furdyna added a comment. > I think its worth mentioning, that my personal preference would STILL be to > land this inside clang-format with default configuration of "OFF", > I would be interested to know how many people would be unhappy if we stated > that "sorting includes" and "namespace

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

HazardyKnusperkeks added a comment. I already said I would like that in `clang-format` and would directly add that to my config. I also think that there should be no problem in having that in `clang-format`, include sorting has a bigger chance of breaking code, yeah only with poorly designed

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2874790 , @MyDeveloperDay wrote: >> so there's something like precedent here > > I think its worth mentioning, that my personal preference would STILL be to > land this inside clang-format with default

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > so there's something like precedent here I think its worth mentioning, that my personal preference would STILL be to land this inside clang-format with default configuration of "OFF", where there is also significant existing precedent for passes that change

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2874464 , @atomgalaxy wrote: > It's very difficult to use a compile_commands database if you can't > actually check out all the code and a remote service builds it for you. In D69764#2874514

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > FWIW, if you use the compile commands database, the only thing you need to do > on the command line is specify the checks to enable or disable. The project I work on doesn't have/use compile commands databases? if you are make based system or some other legacy

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2874404 , @MyDeveloperDay wrote: >> What you're describing sounds like clang-tidy but perhaps with improved >> support for composing source modifications from fix-its, or do you envision >> something rather

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > What you're describing sounds like clang-tidy but perhaps with improved > support for composing source modifications from fix-its, or do you envision > something rather different? All my uses of clang-tidy require extensive command line arguments to handle

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2873225 , @MyDeveloperDay wrote: > If this is something we think we wouldn't want in clang-format, what about > the idea of building a new binary which did allow the modification of the > tokens, This comes up

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. If this is something we think we wouldn't want in clang-format, what about the idea of building a new binary which did allow the modification of the tokens, This comes up from time to time and I kind of feel its a weak argument for those that don't want to use

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > Should any known failure modes be documented? At present, I don't know of any, I think I could probably trick it with macros but then that is #clang-format all over, if you are using it feel free to log bugs in the

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. In D69764#2867109 , @MyDeveloperDay wrote: > In D69764#2863648 , @owenpan wrote: > >> Has this been tested against a large code base? It also needs an unqualified >> LGTM before it can

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. In D69764#2863648 , @owenpan wrote: > Has this been tested against a large code base? It also needs an unqualified > LGTM before it can be merged. D105701: [clang-format] test revision (NOT FOR COMMIT) to demonstrate

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. To be more sure of not breaking something we'd likely have to reduce the cases where tok::identifier was checked, it depends if "not catching every case" is caught is more acceptable. I certainly see that elsewhere in clang (like identifying where override is

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2863312 , @steveire wrote: > In D69764#2863266 , @aaron.ballman > wrote: > >> In D69764#2863213 , @steveire wrote: >> >>>

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. FYI, I'm not actually asking for this to be merged, its just I'm happy to keep rebasing it from time to time for others to use, but to do that I have to keep pushing new reviews, but if the general opinion was to put it in then I'd go for that. I knew it was

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

owenpan added a comment. Has this been tested against a large code base? It also needs an unqualified LGTM before it can be merged. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 ___ cfe-commits

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. In D69764#2863266 , @aaron.ballman wrote: > In D69764#2863213 , @steveire wrote: > >> @MyDeveloperDay Does anything prevent this being merged, instead of just >> rebased? > > Please see

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2863213 , @steveire wrote: > @MyDeveloperDay Does anything prevent this being merged, instead of just > rebased? Please see my comments from https://reviews.llvm.org/D69764#2533538. CHANGES SINCE LAST ACTION

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. @MyDeveloperDay Does anything prevent this being merged, instead of just rebased? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 ___ cfe-commits mailing list

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

0x8000- added a comment. In D69764#2858656 , @MyDeveloperDay wrote: > Rebase on clang12 as requested Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay updated this revision to Diff 356565. MyDeveloperDay removed reviewers: klimek, djasper. MyDeveloperDay added a comment. Rebase on clang12 as requested CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files:

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

0x8000- added a comment. Is it possible to have this rebased on top of LLVM12 for those of us who find the trade-offs acceptable? I prefer a consistent formatting of const placement, and my code base does not have issues with the macros - if we find one that break the build, I'll suggest

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

davidstone added a comment. In D69764#2058590 , @rsmith wrote: > I think that if we are reordering `const`, we should be reordering all > //decl-specifier//s -- I'd like to see `int static constexpr unsigned const > long inline` reordered to something

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a reviewer: djasper. aaron.ballman added a comment. In D69764#2532413 , @steveire wrote: > What can be done to move this change along? Here is my thinking, which is largely unchanged from previous discussion: a code formatting tool

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

0x8000- added a comment. In D69764#2532952 , @sammccall wrote: > In D69764#2532666 , @MyDeveloperDay > wrote: > >>> What can be done to move this change along? >> >> I feel there has to be a fundamental

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

sammccall added a comment. In D69764#2532666 , @MyDeveloperDay wrote: >> What can be done to move this change along? > > I feel there has to be a fundamental acceptance that it is ok for > clang-format to alter code (something it already does with

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

HazardyKnusperkeks added a comment. In D69764#2532666 , @MyDeveloperDay wrote: >> What can be done to move this change along? > > I feel there has to be a fundamental acceptance that it is ok for > clang-format to alter code (something it already does

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. > What can be done to move this change along? I feel there has to be a fundamental acceptance that it is ok for clang-format to alter code (something it already does with sorting of includes, namespace comments). There were fairly strong opinions that

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. The idea has been floated to create a new different tool for changes like this (see eg D95168 ). I don't think this should be done. These kinds of things should be in clang-format. One of the advantages of this and east/west const

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. In D69764#2063798 , @MyDeveloperDay wrote: > @steveire Thanks for the additional test cases, I'm reworking how I handle > the Macro case as I realized that I didn't actually even try to change the > case @rsmith came up with

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. @steveire Thanks for the additional test cases, I'm reworking how I handle the Macro case as I realized that I didn't actually even try to change the case @rsmith came up with in the first place. I made a decision previous that I couldn't handle any case without

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. Here's some more failing testcases. class Aa; class A; struct timespec; // Crash // #define UL unsigned long // Transformed, but with error reported: bool foo(Aa const &); // Not transformed (uppercase) template bool bar(T const &);

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

sammccall added a comment. Random thoughts from the peanut gallery: - `clang-format` *is* the place I'd expect/want this feature, as a user. It's way more similar to `int *x` -> `int* x` than it is to e.g. typical clang-tidy rewrites. My only concern is whether we can give it the safety users

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2058590 , @rsmith wrote: > In D69764#2058334 , @MyDeveloperDay > wrote: > > > @rsmith, Thank you for your comments, I don't agree, but thank you anyway. > > > > I will

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay marked an inline comment as done. MyDeveloperDay added inline comments. Comment at: clang/lib/Format/EastWestConstFixer.cpp:195 +FormatToken *Tok) { + // We only need to think about streams that begin with const. + if

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

curdeius marked an inline comment as done. curdeius added inline comments. Comment at: clang/lib/Format/EastWestConstFixer.cpp:195 +FormatToken *Tok) { + // We only need to think about streams that begin with const. + if

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay updated this revision to Diff 266883. MyDeveloperDay added a comment. That's a rotate! Remove the multiple swap functions for a single rotateTokens function (Thanks for the inspiration @curdeius) extract the various combination of 2,3,4,5 qualifier types to a simple begin and

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay marked 3 inline comments as done. MyDeveloperDay added a comment. In 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

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay marked an inline comment as done. MyDeveloperDay added inline comments. Comment at: clang/lib/Format/EastWestConstFixer.cpp:130 + +static void swapFourTokens(const SourceManager , + tooling::Replacements , curdeius

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay updated this revision to Diff 266799. MyDeveloperDay added a comment. Minor change for the simpler review comments before refactoring some of the more involved ones CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 Files:

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

curdeius added a comment. 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`

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

curdeius added inline comments. Comment at: clang/docs/ReleaseNotes.rst:359 +- Option ``ConstPlacement`` has been added auto-arrange the positioning of const + in variable and parameter declarations to be ``Right/East`` const or ``Left/West`` It sounds

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

rsmith added a comment. In D69764#2058334 , @MyDeveloperDay wrote: > @rsmith, Thank you for your comments, I don't agree, but thank you anyway. > > I will continue to feel there is some value here, My hope is that others will > feel the same. To be

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. @rsmith, Thank you for your comments, I don't agree, but thank you anyway. I will continue to feel there is some value here, My hope is that others will feel the same. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay updated this revision to Diff 266647. MyDeveloperDay added a comment. Begin to cover the cases raised as potential code-breaking changes by ignoring potential MACRO usage add support for more complex `const unsigned long long` cases (I believe I can simplify this to a more

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

rsmith added a comment. In D69764#2057945 , @aaron.ballman wrote: > In D69764#2056104 , @rsmith wrote: > > > I also don't think this is something that a user would *want* in > > `clang-format`: changing the

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2056104 , @rsmith wrote: > I'm uncomfortable about `clang-format` performing this transformation at all. > Generally, clang-format only makes changes that are guaranteed to preserve > the meaning of the source

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

0x8000- added a comment. @MyDeveloperDay +1 from the trenches - I am in that same position and it took a lot of work to get the organization to _align_ on a consistent style, then enforce that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay updated this revision to Diff 266460. MyDeveloperDay added a comment. Fix crash whilst rechecking polly ../polly/lib/Analysis/ScopBuilder.cpp:74:8: warning: code should be clang-format ted [-Wclang-format-violations] static int const MaxDimensionsInAccessRange = 9;

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay marked an inline comment as done. MyDeveloperDay added a comment. @rsmith, firstly let me thank you for taking the time to review this, I didn't realize i'd invoke such a reaction that the big guns would start showing up.. fundamentally I agree with you, but let me defend my

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. In D69764#2056104 , @rsmith wrote: > I'm uncomfortable about `clang-format` performing this transformation at all. > Generally, clang-format only makes changes that are guaranteed to preserve > the meaning of the source

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

rsmith added a comment. I'm uncomfortable about `clang-format` performing this transformation at all. Generally, clang-format only makes changes that are guaranteed to preserve the meaning of the source program, and does not make changes that are only heuristically likely to be

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay updated this revision to Diff 266360. MyDeveloperDay added a comment. rebase with master 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 East/West Const fixer capability

MyDeveloperDay marked 8 inline comments as done. MyDeveloperDay added inline comments. Comment at: clang/lib/Format/EastWestConstFixer.cpp:139 + return (Tok->isSimpleTypeSpecifier() || + Tok->isOneOf(tok::kw_volatile, tok::kw_auto)); +} aaron.ballman

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay updated this revision to Diff 266358. MyDeveloperDay marked an inline comment as done. MyDeveloperDay added a comment. Fix issue when preprocessor #if/#else is present Rename the config file name to `ConstPlacement` change the command line option to be `--const-placement` Add

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added inline comments. Comment at: clang/lib/Format/EastWestConstFixer.cpp:22 + +#define DEBUG_TYPE "using-declarations-sorter" + Should this be removed? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added a comment. In D69764#2055716 , @MyDeveloperDay wrote: > I really do appreciate the reviews and the comments especially regarding > east/west/left/right/wrong ;-), I know there won't be 100% agreement, but I > think most people who

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. > if I put any declarations inside the preprocess clauses they actually don't > get converted. Sorry, I'm not certain what this means. Does it mean that if you have #if 0 Foo> P; #else Foo> P; #endif that neither of them get converted? Can you point me

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay added a comment. I really do appreciate the reviews and the comments especially regarding east/west/left/right/wrong ;-), I know there won't be 100% agreement, but I think most people who even consider clone the LLVM repo know what we mean by East/West. as such I'm going to

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

MyDeveloperDay marked an inline comment as done. MyDeveloperDay added inline comments. Comment at: clang/lib/Format/Format.cpp:2547 + if (Style.isCpp() || Style.Language == FormatStyle::LK_ObjC) { +if (Style.ConstStyle != FormatStyle::CS_Leave)

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

steveire added a comment. @MyDeveloperDay Thanks for the update. I pinged you on slack about this, but I guess you're not using it at the moment. I asked if you have a git branch somewhere with this change. Downloading patches from phab is such a pain I have no idea why we use it. If you can

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

aaron.ballman added inline comments. Comment at: clang/lib/Format/EastWestConstFixer.cpp:139 + return (Tok->isSimpleTypeSpecifier() || + Tok->isOneOf(tok::kw_volatile, tok::kw_auto)); +} Do you need to look for `restrict` here as well as `volatile`?

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

I prefer east/west, but I think there's a terminology that is uncontroversial (left/right) and one that is controversial. It's also clear to me that it's better to have only one set of terms (aliases are only fine for backwards compatibility). Left/Right won, I think. On Tue, May 26, 2020 at 1:55

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

sammccall added a comment. Thanks @MyDeveloperDay for hammering on on these bugs and @steveire for finding them! There's still obviously some risk here but as long as this is opt-in for a major release or two (i.e. not turned on in built-in styles, any remaining bugs should get flushed out.

  1   2   >