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 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

Yay, thanks!

I am surprised this works as well as it does, but tests seem to indicate it does, so... LG (minus potentially reducing some duplication :)

So, I did like the more exhaustive doc, I thought you'd move it to the code so it also shows up in the generated doc page :) (sorry for not being clearer here)

LG, thx!

In D64297#1572872, @MaskRay wrote:

Is distcc gcc.exe or gomacc gcc.exe possible?

The design of the compilation database/plugin model makes it pretty hard to add options.

Yes. It also lacks user settings. Downstream compile_commands.json consumers may provide their own command line option filtering mechanism that users can customize and add more compiler schedulers. For some tools, they probably don't want to see filtering applying on multiple layers, especially if the filtering mechanism done at the JSONCompilationDatabase layer may have false positives.

LG

LG

In D37813#1512317, @Typz wrote:

The patch goal is indeed to indent the content of namespace-macro blocks like the content of any 'regular' namespace. So it should look like the content of a namespace, possibly depending on the choose style options. To sumarize, here is a detailed summary of the observable differences between macros vs normal namespace.

Without this patch, clang-format does not understand the macro call actually defines a namespace, thus:

• the content of the namespace-macro block is indented, even when Style.NamespaceIndentation = FormatStyle::NI_None ` lang=cpp TESTSUITE(A) { int i; // <--- should not be indented } namespace B { int j; } `
• similarly for nested namespaces, when Style.NamespaceIndentation = FormatStyle::NI_Inner : ` lang=cpp TESTSUITE(A) { TESTSUITE(B) { int i; // <--- should be indented 2-chars only } } namespace C { namespace D { int j; } } `
• There is no automatic fix of namespace end comment when Style.FixNamespaceComments = true ` lang=cpp TESTSUITE(A) { int i; } <--- should have a namespace end comment namespace B { int j; } namespace B `
• Multiple nested namespaces are not compacted when Style.CompactNamespaces = true ` lang=cpp TESTSUITE(A) { TESTSUITE(B) { <--- should be merged with previous line int i; } } <--- should be merged with previous line (and have a "combined" namespace end comment) namespace A { namespace B { int j; } // namespace A::B `
  
  ---
  
  This patch fixes all these points, which hopefully leads to the exact same formatting when using the namespace keyword or the namespace-macros: ` lang=cpp // Style.NamespaceIndentation = FormatStyle::NI_None TESTSUITE(A) { int i; } namespace B { int j; }
  
  // Style.NamespaceIndentation = FormatStyle::NI_Inner TESTSUITE(A) { TESTSUITE(B) { int i; } } namespace C { namespace D { int j; } }
  
  Style.FixNamespaceComments = true TESTSUITE(A) { int i; } TESTSUITE(A) namespace B { int j; } // namespace B
  
  Style.CompactNamespaces = true TESTSUITE(A) { TESTSUITE(B) { int i; }} TESTSUITE(A::B) namespace A { namespace B { int j; }} // namespace A::B `
  
  I did not see any issue in my testing or reviewing the code, but if you see a place in the tests where it does not match this, please point it out !

In D37813#1254891, @Typz wrote:

In D37813#1254865, @klimek wrote:

In D37813#1253813, @Typz wrote:

In D37813#1184051, @klimek wrote:

The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them not actually be formatted exactly like namespaces or classes, I think we need a more generic mechanism for you to express that.

Not sure what you mean here. I want them to behave like namespaces as well, this is actually the use case I have... As implemented, they indeed behave exactly like namespaces :

TESTSUITE(a) {                       namespace a {
} // TESTSUITE(a)                    } // namespace a
                                VS
TESTSUITE(a) { TESTSUITE(b) {        namespace a { namespace b {
} // TESTSUITE(a::b)                 }} // namespace a::b

I thought we had the discussion upthread that they indent differently from namespaces. I'm working on a patch that allows you to configure macros.

The current behavior is that they indent differently from namespace, because clang does not know these macros are conceptually equivalent to namespaces. And this patch actually fixes that, letting clang know they are actually macros.

lg

In D54881#1462893, @MyDeveloperDay wrote:

I agree and would be happy with the change if it would only change the line-filtered workflow, but this afaict (unless I'm missing something :) will also affect the workflow where the provided range is 0-length range, which has an implicit "format stuff around this" request from the user inside it. I'd be happy with a patch that differentiates these two sides.

This use case needs capturing in a unit test..it isn't obvious

In D54881#1462804, @russellmcc wrote:

Thanks for the explanation! I do understand your philosophy on this, and agree with the desired behavior case you brought up where you have put in new braces.

After thinking about this more, the thing I really care about is that clang-format is idempotent with a line filter - i.e., running it twice should always have the same effect as running it once.

So, either this fix, or your proposed fix of fixing all lines until the next correct indentation would meet that idempotence criteria.

However, I think in this particular case I still prefer my fix - to me, line filter is meant to limit the effect of clang-format to just fix a particular change and the fallout from that. However, if the lines _after_ a change were wrong before, this feels very unrelated to the change that was made - why is now the time to fix these unrelated lines?

In D54881#1449848, @russellmcc wrote:

klimek: I'm sorry, I don't fully understand your proposed fix. Could you explain it in more detail?

Without being able to respond to your proposal in detail, I strongly believe if you pass in a line range to clang-format, it should not change lines outside that range OR we should modify the documentation to clearly explain what the line filter option does.

In D59746#1458539, @hintonda wrote:

In D59746#1458461, @hintonda wrote:

In D59746#1458432, @klimek wrote:

If we make it an alias by default, can somebody overwrite that?

Unfortunately, that produces a runtime error:

lan1:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ bin/llvm-objdump -h
: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

The operative lines:

llvm/tools/llvm-objdump/llvm-objdump.cpp:187:static cl::alias SectionHeadersShorter("h",
llvm/lib/Support/CommandLine.cpp:2149:static cl::alias HOpA("h", cl::desc("Alias for -help"), cl::aliasopt(HOp));

The other problem is that these are statics, so you can't count on the order, i.e., did the user overwrite get processed before or after the one in CommandLine.cpp?

In D59746#1458424, @hintonda wrote:

In D59746#1458086, @klimek wrote:

In D59746#1440756, @hintonda wrote:

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

Is that intentional? Can you point at samples?

A quick grep found these -- there could be more.

llvm-opt-report adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-objdump adds -h as an alias for --section-headers.
dsymutil adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-readobj adds -h as an alias for --file-headers.
llvm-dwarfdump adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-mt rolls its own via llvm-tblgen.

So, the only ones I found that aren't essentially aliases for help are in llvm-objdump and llvm-readobj.

In D59746#1440756, @hintonda wrote:

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

Apart from the typo I think this is a simple enough change and a widely enough used style that it LG.

lg

+1 to quite some overlap in the tests. I'd have expected three test cases:

1. ; line comment
2. ; C-comment
3. no semi

The previous behavior looks intentional, and much more regular. I'd be curious why you think the proposed behavior is more readable.

lg

I just verified why this doesn't break for our CI:
The trick is that if the indent is actually off (as opposed to being correct, but tab vs spaces), we do re-indent until we find an indent that's correct.
The fix that would make me happier, I think, is to do the same thing here:
Basically, when we expand the range until nothing changes, do not only check whether the indent is correct, but also whether we'd change the spaces in the indent.
WDYT?

lg

LG

Minus me understanding the TT_TemplateString change, the rest looks nice now, thanks!

Oh, and LG :)

In D55170#1439864, @MyDeveloperDay wrote:

@lebedev.ri Are we talking about a general ideology of the long term cost to allow any new things in? or to not allow things in this specific case?

because in this specific case all the changes are based on what is really a single clause that was already there before, namely

Style.SpaceBeforeParens == FormatStyle::SBPO_Always

Now that clause is abstracted away to be (the abstraction in my view being a nice part of this change, becuase it avoided duplication and made it easier to reason about!)

Style.SpaceBeforeParens == FormatStyle::SBPO_Always  ||
(Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses && Right.ParameterCount > 0);

i.e. always add a space, or add a space only when the number of parameters is > 0 and I don't want a space on empty parameters. (simple enough!)

So whilst I understand the arguments of code complexity, I feel it is sometimes used to discourage, what is in at least my view seemingly reasonable requests. (not my choice but reasonable all the same)

This was why I asked what was meant by "cost", but you raise the other cost, and that is one that is more legitimate... or is it?. have we seen it here as a problem or do we just fear it?

LG

LG. This looks pretty sharp (scnr).

Great idea! LG from my side after addressing MyDeveloperDay's comments.

I don't understand yet why running clang-format locally would not do the same change?

lg

In D55170#1432929, @MyDeveloperDay wrote:

I don't think risk is what matters - in the end it's about cost, and cost is a very technical reason

I'm really sorry I'm not trying to be difficult its just over the years I keep seeing this being said in reviews? but what is the cost? I assume you don't mean a financial cost? could you define cost as a problem so we can address it? what would it take to make cost not an issue?

In D55170#1432832, @reuk wrote:

@klimek I agree that the rule is somewhat arbitrary. However, it's the style rule of an established codebase with many users (I don't have a concrete number, but the project has 1400 stars on github). I've found this patch useful when contributing to JUCE and I thought others might too.

In D55170#1431854, @MyDeveloperDay wrote:

I'm sorry for not having a positive answer here, but I'm not in favor of this change. The style rule looks like it introduces arbitrary complexity at a point where I don't understand at all how it matters. We cannot possibly support all style guides that come up with random variance. I do not think this option pulls its weight.

So is this a personal opinion or based on a technical reason (other than the normal mantra about risk & cost),

In D40988#1430502, @MyDeveloperDay wrote:

So @russellmcc you've been bumping along this road nicely for 6 months, doing what people say... pinging every week or so in order to get your code reviewed, and you are getting no response.

Was there anything you think people were objecting too other than the normal "its a high bar to get in" and "its complex in here"?

I think its fairer to the person submitting a revision if they don't want feature in, we should give feedback as to why, but are we to assume silence is acceptance? how long do we wait? who is the decision maker?

I've personally never met an Engineer who didn't like having more knobs to fiddle with so I don't believe the rational that having more options is bad as long as they don't interfere with each other, for that there is the "Beyonce rule", if adding an option breaks someone else then "if they liked it they should have put a test on it!"

As far as I can tell this LGTM (I'm not the code owner, just someone wanting to help)

In the meantime I have uploaded this patch to my fork, where I'm maintaining a clang-format with revisions that seem ok, but get stalled in the review process.

https://github.com/mydeveloperday/clang-experimental

Yea, the
a b(c)
problem is the olden C++ problem clang-format will never be able to solve, thus all these crazy heuristics.

In D59309#1428969, @MyDeveloperDay wrote:

That works because the argument list is just tok::identifier and TT_StartOfName

IIRC this was done due to performance problems in large nested initializer lists. Can you verify this doesn't regress performance?

Given this doesn't add an extra option (but only an extra enum) and is fairly straight forward, LG.

In D28462#1406716, @jacob-keller wrote:

In regards to whether clang-format should support this feature, I think it's pretty clear that a large number of users want it to. The tool already supports formatting various other definitions in a similar manner, including variables and struct members. So I don't see how a similar features which aligns consecutive macros is something which doesn't make sense.

Additionally, because the formatting will *undo* such formatting if done manually, it means that the existing source code formatting this way cannot be handled via clang-format. In my case, it essentially means that I cannot use clang-format to enforce the style guidelines, as the macros definitions are aligned.

Additionally, aligning a series of macros makes sense because it helps improve readability, and is thus something that I'd rather not lose if I were to switch to using clang-format without the feature.

Can you provide some evidence:

• that this is in a style guide that's used widely enough
• why you believe this matters

In D55170#1423941, @MyDeveloperDay wrote:

In D55170#1423798, @reuk wrote:

Thanks for the review, and for approving this PR. It's very much appreciated!

I don't have merge rights - could someone commit this for me please?

I'd actively encourage you to go get commit access, I think its much better when the commit comes from the author.

There just isn't enough people actively involved in clang-format (and other tools) for us to get reviews even looked at (even for defects), we need more people involved fixing defects and doing reviews, getting commit access shows an intent to fix anything in that comes from your own review which is one of the things the code owners push as being an objection to adding more code.

In D59309#1428807, @MyDeveloperDay wrote:

In D59309#1428799, @klimek wrote:

Why did the numeric constant break this? Which path would the function run through?

as its looking though the parameter list is doesn't recognize a numeric constant as being a valid thing it would see in an argument list and so falls through the bottom to return false, it will only be where there is a numeric_constant as the first argument, if it has a second argument and that argument is just a simple type it would still fail, but switch them around and it won't

As I look more at this, even this would (and still would) fail to break

int TestFn(A=1)
{
  return a;
}

originally I didn't have the counting of the <> but one of the unit tests failed where we had

verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
             "    LoooooooooooooooooooooooooooooooooooooooongVariable(1);");

I guess they wanted to do something like this when the type and name is very long

int
variable(1);

but I think this gets thought of as being a potential FunctionDecl

so I held off just adding tok::numeric_constant to the isOneOf(), and went with the use case

I think perhaps I could solve the default argument by adding a

startsSequence(tok::equal, tok::numeric_constant)

In D33029#1423949, @MyDeveloperDay wrote:

In D33029#1423947, @lebedev.ri wrote:

In D33029#1423944, @MyDeveloperDay wrote:

Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.

I personally don't like to see revisions like this sit for 2 years with nothing happening, I don't see anything wrong with this that would prevent it going in so I don't understand whats blocking it?,

if you had some tests and a release note I'd give it a LGTM (but as I've said before I'm not the code owner, but someone wanting to address defects and add capabilities. but I think we need to be able to move forward, people will object soon enough if they don't like it.)

Generally I don't understand why clang-format is so reluctant to change anything, As such we don't have many people involved and getting anything done (even defects) is extremely hard.

It looks like you met the criteria, and reviewers have been given ample opportunity to make an objection. the number of subscribers and like tokens would suggest this is wanted,

Please also add a line the in the release notes to say what you are adding. In the absence of any other constructive input all we can do is follow the guidance on doing a review, for what its worth I notice in the rest of LLVM there seems to be a much larger amount of commits that go in even without a review, I'm not sure what makes this area so strict, so reluctant to change especailly when revisions do seem to be reviewed.

I don't have any stake here, but i just want to point out that no tool (including clang-format)
will ever support all the things all the people out there will want it to support. But every
new knob is not just a single knob, it needs to work well with all the other existing knobs
(and all of the combination of knob params), and every new knob after that.

It's a snowball effect. Things can (and likely will, unless there is at least a *very* strict testing/quality policy
(which is https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options about))
get out of hand real quickly..

Just 2c.

Correct we should always be adding tests and show we don't break existing tests... We need to apply the "Beyonce Rule".. "if you liked it you should have put a test on it."

We shouldn't just give up making improvements or fixing defects because something is hard or complex.

Why did the numeric constant break this? Which path would the function run through?