Noticed I should have waiting with the renaming of the file until the review is done :( Sorry for the extra confusion.

Work in review comments.

Thanks for cleaning up after me, and sorry for the mess - do y'all have clang-format set up as a presubmit or do you just remember to format manually?

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 '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 contributions are immense, and his points are too-well-reasoned and motivated to be dismissed.

Just so we are clear I'm not dismissing anyone opinions, @arron.ballman and I have been going at this quite a bit both on and off list. I have huge respect for these people, (even if the defence of my review it might not seem so). I can't even think to emulate their contributions.

Ideally I'd like their blessing, but alas I fear that might be beyond my capabilities, but if there are things like the RFC that could even go some way to potentially them seeing a way forward that is mutually beneficial so that the door is even open a jar for this then I'm happy to try.

If ultimately the answer is "no" then I need to understand what can be done if anything, if that means a new tool then I'm ok with that as the compromise. Out right dismissal, I would be very sorry to see.

Here are my thoughts put in one place.

0) I like the idea of being able to format qualifier location, and I think clang-format is the tool I would reach for to perform that work
.33) I wish this was generalized to handle all qualifiers rather than just const. (This is not a blocking wish.)
.66) I wish this used "left" and "right" rather than "east" and "west" for user-facing options and documentation. (This is Very Strong wish but I won't block the patch over it.)

1. In general, I do not like the idea of my code formatting tool having opinions about the input source's existing formatting (I think valid code should remain valid). This is the area of concern causing me to ask for an RFC because I'm operating under the impression that not breaking code is (generally) the status quo for clang-format.
2. If the community consensus is that formatting can break code (blanket permission, case by case basis, whatever), I'll definitely abide by that decision. I just want it to be an explicit decision from a wider audience than people who might be following this very long-running patch.
3. The proposed opt-in nature of the check is a blessing and a curse. It alleviates some of the danger (it's not dangerous by default, you have to manually say you want it). But it doesn't eliminate the danger (code can still be broken) and it does raise the question of how often people opt into this sort of thing and whether it's worth the maintenance costs. I don't think any of us can answer those "do people opt in" questions though, so that's not a concern I think we should hold anything up over unless someone has actual data on usage of opt-in features for clang-format. I think opt-in alleviates enough of the danger that it's a worthwhile approach for this patch (and likely may be a good idea for a policy on future changes of the same kind).

tl;dr: if there's an RFC and the community says "breaking changes aren't that big of a deal to us", it tips me from "against" to "in favor" for this functionality.

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, especially as this functionality is off by default (and needs to stay that way). This was my suggestion for a compromise.

Its a tough call, but ultimately no on ever gave this a LGTM so I couldn't proceed anyway. Whilst I think we can cover the other qualifiers with a similar approach I don't want to waste more of my time until the fundamental question has been answered if its ok for clang-format to insert/modify the order of tokens and that is more a collective decision.

It was suggested to me that I write up and RFC on the matter, I'm not a massive fan of those as I don't really see a formal process for handling them (the conversation seems to turn into a series of personal preferences then dwindles and dies without conclusion). But if people think it would solve something I guess I could try.

From my perspective I'm more interested in the views of the major clang-format contributors (@curdeius, @krasimir, @owenpan , @sammccall, @HazardyKnusperkeks and people like yourselves who have put time and effort into blogging about these tools), ultimately it will be us who have to handle the impact of this (and maybe the #clangd people) and I don't want to inconvenience them or make work for them.

So here is what is needed to land this in my view:

1. I'd want at least 2 to 3 of the current reviewers to LGTM.
2. I'd want 1 person (like yourself) a community contributor to also give their approval (I guess I have that conceptually from you!)

or

1. @klimek or @djasper to give us an its ok for "clang-format to insert/modify the order of tokens" in a controlled manner decision
2. 1 LGTM

Whilst I respect the views of those few who have objected, they don't to my knowledge make significant contributions to clang-format (not in the last 5-6 years I've been following it anyway)

I feel this team owns the decision as we are the ones that look after it, warts and all. We'll always have a situation where some don't agree, thats ok, but I feel there is more agreement that this funcationality is ok rather than not.

Until one of those 2 things happens, we are in limbo.

Can you explain the change and the before/after a bit more? Thanks!

Isn't the comment incorrect after this patch?

Adapted based on review comments.

lg

Nice catch, thanks! LG!

In D83296#2299062, @nridge wrote:

What does this change mean for users of clang-format -- better formatting of complicated (e.g. multi-line) macro invocations?

In D84306#2294952, @MyDeveloperDay wrote:

Which bit do you find more complex? adding something to the FormatToken or the use of the is() calls?

FWIW, finding this due to seeing the added complexity. Do you have data on what the difference is on clang-format for either overall memory use or performance? Thanks!

Worked in review comments.

Addressed code review comments.

In D83296#2146970, @sammccall wrote:

In D83296#2146870, @klimek wrote:

Monday-morning ping.

Thanks for the reminder here... however this is taking me a bit to get my head around, and we've got a release branch cut scheduled for a couple of days that we're trying to polish for.
AFAICT there's significant followup work still needed to make use of this - are you wanting this to land in the 11 release? Else i'd probably come back to this after the cut...

Monday-morning ping.

@djasper wrote this iirc, but I doubt he'll have much time to look into this :)

Address review comment.

lg

LG

In D82771#2121924, @hokein wrote:

In D82771#2120214, @klimek wrote:

In what situation are we calling child matchers repeatedly with the same matcher on the same node?

I guess a pattern like https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp#L29-L40?

In what situation are we calling child matchers repeatedly with the same matcher on the same node?

+Sam for additional sanity checking.

General note on that patch:
I am fine with both views, I know people who like to use arc land, I know people (like myself), who like to not have noisy messages.
I don't know that we have a good process to really decide that (Chris as BDFL dropped himself out of this one :)
I think giving folks a arcfilter line like Mehdi did is a great idea if we decide to go with the policy.

In D80961#2099262, @steveire wrote:

In D80961#2095254, @klimek wrote:

1. the scare quotes around "standing objections" reads like you're not respecting the opinions of others here;

Hmm, this wasn't intended. I sometimes quote things if they are a particular term (ordinarily I would quote "particular term").

Nevertheless, I'll take on your message. I don't have more to say.

In D80961#2090216, @steveire wrote:

In D80961#2079419, @aaron.ballman wrote:

In D80961#2079044, @klimek wrote:

In D80961#2076242, @aaron.ballman wrote:

In D80961#2073049, @klimek wrote:

Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.
Having to put AsIs on a matcher every time you need to match template instantiations is a rather big change (suddenly you have to change all the matchers you've written so far).

I think that's the intended meaning of AsIs though. Template instantiations are not source code the user wrote, they're source code the compiler stamped out from code the user wrote. I hope IgnoreUnlessSpelledInSource isn't a misnomer.

I love the idea of being able to control visitation of template instantiation.
I am somewhat torn on whether it should be the default, and would like to see more data.
I feel more strongly about needing AsIs when I want to match template instantiations.

FWIW, my experience in clang-tidy has been that template instantiations are ignored far more often than they're desired. In fact, instantiations tend to be a source of bugs for us because they're easy to forget about when writing matchers without keeping templates in mind. The times when template instantiations become important to *not* ignore within the checks is when the check is specific to template behavior, but that's a minority of the public checks thus far.

So I assume the idea is that this will work & be what we'll want people to write?
traverse(AsIs, decl(traverse(IgnoreImplicit, hasFoo)))

I believe so, yes. It's explicit about which traversal mode you want any given set of matchers to match with, so it is more flexible at the expense of being more verbose. Alternatively, you could be in AsIs mode for all of it and explicitly ignore the implicit nodes you wish to ignore (which may be even more verbose with the same results, depending on the matchers involved).

LG. Thanks!

We have some precedent for overloading has* matchers, but I'll defer to Sam's judgement whether that's a good idea here.

In D80721#2079534, @ilya-biryukov wrote:

@sammccall, @klimek, @gribozavr2, @MyDeveloperDay who would be the best reviewer for this?
Could do the reviews myself for small CLion-related documentation changes if everyone is ok with this. Does that LG?

In D80961#2076242, @aaron.ballman wrote:

In D80961#2073049, @klimek wrote:

Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.
Having to put AsIs on a matcher every time you need to match template instantiations is a rather big change (suddenly you have to change all the matchers you've written so far).

I think that's the intended meaning of AsIs though. Template instantiations are not source code the user wrote, they're source code the compiler stamped out from code the user wrote. I hope IgnoreUnlessSpelledInSource isn't a misnomer.

I love the idea of being able to control visitation of template instantiation.
I am somewhat torn on whether it should be the default, and would like to see more data.
I feel more strongly about needing AsIs when I want to match template instantiations.

FWIW, my experience in clang-tidy has been that template instantiations are ignored far more often than they're desired. In fact, instantiations tend to be a source of bugs for us because they're easy to forget about when writing matchers without keeping templates in mind. The times when template instantiations become important to *not* ignore within the checks is when the check is specific to template behavior, but that's a minority of the public checks thus far.

Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.
Having to put AsIs on a matcher every time you need to match template instantiations is a rather big change (suddenly you have to change all the matchers you've written so far).
I love the idea of being able to control visitation of template instantiation.
I am somewhat torn on whether it should be the default, and would like to see more data.
I feel more strongly about needing AsIs when I want to match template instantiations.

In D80961#2068865, @ymandel wrote:

Thank you for bringing up this issue. I think it highlights an underlying problem -- editing templates is quite difficult -- that neither setting will address, as Dmitri expanded on above. Given the parallel to macros, I'd say your change is better than the status quo. Most clang tidies and other rewriting tools that I've encountered simply skip code in macro expansions, rather than reason about how to update the macro definition or whatnot. So, by that reasoning, we should skip template instantations.

For the policy question: clang-format does intentionally not try to be stable - the "how to" suggestion for clang-format has always been to format changes lines and live with divergence (divergence from people manually formatting things is larger).

In D80202#2046321, @njames93 wrote:

In D80202#2046266, @klimek wrote:

Given the extra complexity I'd like to see that it matters - bound nodes tend to be small.

I put that in the description, but this is where i need help. whats the best way to benchmark the matchers?
Also, do you know how it was benchmarked when MaxMemoizationEntries was decided upon?
There was also comments about some making some micro benchmarks but I don't think that was acted upon.

Given the extra complexity I'd like to see that it matters - bound nodes tend to be small.

Update docs.

Rebase.

Adding Sam, as I remember clangd having that problem.

lg

I'm not generally opposed to this, given that
a) clang-format already changes code; I think by now we're not fixing double semicolon mainly for workflow reasons, would be fine to add
b) the implementation is very self contained

lg

generally makes sense

since resource directory should be picked relative
to the path of the clang-compiler in the compilation command.

lg

lg

In D68969#1709696, @MyDeveloperDay wrote:

In D68969#1709321, @klimek wrote:

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

I may need to, it feels slow for large files which I suspect is in the splitting, which will be mostly unnecessary.

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

In D68914#1709002, @owenpan wrote:

Here are some [[ https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp#L229 | const StringRef & examples ]].

In D68554#1707540, @thakis wrote:

Could you or anyone else point me to a good example, I'd definitely like to take a look.

http://llvm-cs.pcc.me.uk/include/llvm/Support/SourceMgr.h/rSMDiagnostic --

`via clang-format?` did you mean via clang.exe or clang-check.exe?   I did think of this when I started doing this work, I just came to the conclusion that if I did that it wouldn't be long before someone requested the same features in clang-format itself.

Err, via clang-tidy. Sorry, that was a super confusing typo. That seems like a great place for this feature to me.

lg

In D68554#1703358, @MyDeveloperDay wrote:

I think it's easy enough to do as a kind of driver, either in python or C++, but then again, at that point putting it into ClangFormat seems fine. One thing that I find on the one side kinda nice, but on the other side also weird is the names of the flags. Making them more similar to clang flags is nice from the point of people used to clang being able to quickly identify them, but the structure also makes it look like clang-format would support a wider range of flags, which it doesn't. Not sure which side I'm on, and I'm fine with the patch as is (minus pulling out a function), but wanted to bring it up if folks have ideas.

The reason I added -Wclang-format-violations and -Wnoclang-format-violations was not only because I wanted to treat them like compiler warnings, but also so that the command line argument could be used the turning on/off to alter the exit code so as to either stop a build or continue based on -Werror or if you'd turn the individual warning off.

Also I know there are external tools, (I think SonarCube) which I think could read the code in the [] e.g. 'ClangFormat.cpp:54:29: warning: code should be clang-formatted [-Wclang-format-violations]' and be able to use that along with the file, row and column information to categorize and the annotate source code.

if you think this is too much perhaps I should make a new tool clang-format-check that did only this, but then I am concerned that it would be almost 100% clang-format anyway, and it wouldn't be long before a request came in to merge the functionality.

In D68554#1702090, @MyDeveloperDay wrote:

As this behaviour is not something clang-format should do because there are external tool that can do this for us, I thought I'd set myself a challenge of trying to write this with external tools, to prove to myself that it was indeed easy, what I want to achieve is being able to go to a directory and determine if there are any clang-format changes needed in the source files of the levels below with a single command.

The idea would be that this could be something that could fit on the command line, or made into a bash alias/function (more likely), I really don't want to turn to python as if I'm coding this in another language I might as well be coding it in C++ inside clang-format and it opens me up to the whole can't find the correct python headache. (but I also think the clang diagnostic classes give doing it inside C++ the edge)

I want to take into account at least the concepts from D29039: [clang-format] Proposal for clang-format -r option about being able to look deeply into a directory.

There are a couple of requirements: (some nice to have's)

1. look deeply into a directory for all files supported by clang-format (C++,C# etc)
2. file extensions should be case insensitive i.e. .cpp or .CPP
3. be able to clearly see the name of the file that need clang-formatting
4. don't actually do the formatting (tell me what needs doing but don't necessarily make the change)
5. ideally show a snippet of the file where the change is needed (I guess this isn't essential)
6. This should be cross platform windows, linux, (I guess other *nix's) it should work in at least the common shells bash,cygwin,powershell

The cross platform is the real killer, this forces you down the python road, (or the C++) if you want a common solution otherwise you are implementing different solutions on different plaforms.

But lets ignore that and lets start with a Linux or Cygwin bash shell. Surely the best way to find all source files that can be converted is via a find. In the past I've only ever used find like this to find multiple extensions

find .\( -name "*.h" -o name "*.cpp" \) -print

But this is really unworkable for so many output formats that clang-format supports, and even if you could get that to work it would be case sensitive unless you uses -ilname which would make the command line even longer

So lets try using -iregex, this is a much shorter case insensitive command but still, it needs a certain amount of escaping in the regular expression, (lets hope we never do "clang-format all the things") as this could get longer.

find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -print

if I want to put that into an alias then I need switch the ' and " around

alias find_all_code='find . -iregex ".*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)" -print'

Is the same thing as easy in powershell? Ok not really .. this is as far as I got, and I couldn't work out how to handle multiple extensions let alone case sensitivity.. (it feels very slow too!)

PowerShell.exe -Command "dir -Recurse -ea 0 | ? FullName -like '*.cpp'

So at least my brief initial conclusion is... if we are not using C++, its Python or nothing if you want cross-platform or maybe find if you only care about *nix

But even the find... that only covers 1) and 2) of my requirements...how do I determine even with find which files need formatting..

This command would show me the contents of everything that needs changing.

find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format {} \;

This would give me the list of changes, but then I lose the name of the file.

find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format --output-replacements-xml {} \;

So using external tools is easy how? the dumping of the output to stdout or the replacement xml to stdout makes this much harder as I lose the filename that was passed to clang-format, (there is probably an xargs trick here I'm missing)

This proposed solution would make all this possible just with:

find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format -i -n {} \;

and more to the point the list of file types supported by clang-format is something clang-format knows... why not combine this with D29039: [clang-format] Proposal for clang-format -r option and just do

clang-format -r -i -n .

This provides in my view the only cross platform, shell independent way of doing this in what is a relatively minimal code change, and I'm struggling to see this as easy to do with an external tools? but if someone wants to show me how, I'd be interested to learn.

Why not build a tool that takes the diff output of clang-format and changes it to what you want? Wouldn't that be a couple of lines of python?

In D68296#1691393, @mitchell-stellar wrote:

Thanks for the review. I have added documentation updates.

I do not have a public style guide to reference. My company just switched to auto-clang-formatting all of our code, and this patch addresses one of our biggest pain points: readability. When the opening brace of a multi-line control statement is not wrapped, we simply cannot tell at first glance where the control statement ends and where the body begins due to our indentation settings.

I think the fact that you yourself see the benefit of this change speaks to its viability.

I cannot think of any other brace wrapping rules that need this level of control. Our code base consists of millions of lines of code, so I'm pretty sure we would have found another instance.

I would be willing to support this patch and further revisions if this gets into master, as our company will depend on this feature.

LG, nice patch, thanks!

In rG82aaf17412134fcbf158b97ffec2f6344ee262ea#691434, @MyDeveloperDay wrote:

@klimek I'm wondering if we should be thinking about including .ixx and .mxx files (or other common modules files names)

lg

LG

LG. That makes lots of sense now :) Thanks!

LG