In D56841#1361395, @kadircet wrote:

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

Just in general, I'd like to add that my experience over the years dealing with folks trying to do AST matchers is that the inability to express something is much more of a problem than matching too much, simply because the test cases people think of are usually small, and when running a transformation on a very large codebase, the cost of false negatives is significantly higher than the cost of false positive: the cost of a false negative means that you may produce something incorrect. The cost of a false positive is that you adapt your matcher to exclude the false negative.

In D56444#1351056, @aaron.ballman wrote:

In D56444#1350849, @JonasToth wrote:

In D56444#1350746, @sammccall wrote:

@klimek: would it be better to preserve the odd behavior of the functionDecl() matcher, and add a new functionOrLambdaDecl()? It seems too surprising to me.

We can change the clang-tidy check as well. I think lambdas should be considered functionDecls, shouldnt they? WDYT @aaron.ballman ?

I think it depends on whether we want the AST matchers to match what the user wrote (syntax) or how the program behaves (semantics). If the user writes a lambda, they did not syntactically write a function declaration and so it makes sense for functionDecl() to not match. However, the semantics of a lambda are that they act as a functor (in effect) which includes a class declaration, a function declaration for the function call operator, conversion operators, etc and so it does make sense for users to want to match those semantic components. Given that, I kind of think we should have functionDecl() match only functions, and give users some other way to match the semantic declarations in a consistent manner. Alternatively, we could decide semantics are what we want to match (because it's what the AST encodes) and instead we give users a way to request to only match syntax.

In D56444#1350746, @sammccall wrote:

In D56444#1350252, @JonasToth wrote:

I still see the unit-test crashing for ExprMutAnalyzer (just apply the last two tests https://github.com/JonasToth/clang/blob/fix_crash/unittests/Analysis/ExprMutationAnalyzerTest.cpp and run check-clang-unit)

The ReproduceFailure11 test passes for me, and the ReproduceFailureMinimal fails to compile (a&&).

The clang-tidy check does not crash anymore, but readability-function-size regresses. It probably doesn't properly count for lambdas anymore.

Rather, it previously didn't count lambda bodies as functions (because functionDecl() didn't include them), and now it does :-)
I think counting them is better behavior, but I've modified the matcher to preserve the behavior for now.

@klimek: would it be better to preserve the odd behavior of the functionDecl() matcher, and add a new functionOrLambdaDecl()? It seems too surprising to me.

In D54309#1326852, @JonasToth wrote:

See PR39949 as well, as it seems to trigger the same or a similar problem.
@ioeric and @klimek maybe could give an opinion, too?

LG

Thanks Aaron for volunteering, I'm very thankful for all your work on the reviews, it's much appreciated :D

In D54077#1288404, @ioeric wrote:

In D54077#1288387, @sammccall wrote:

Someone mentioned to me that the interaction-between-features argument wasn't clear here:

• we don't currently update diagnostics for A.cc when A.h is edited
• we should, this seems more obvious & important than what we do with drafts
• this interacts badly with using draft state, as this patch proposes - there are too many edits

FWIW, one of my "pain points" when using vim+clangd is:

• Edit A.h in a buffer (and forget to save)
• Switch to A.cc in another buffer
• Realize that I forgot to save A.h
• Go back to save A.h
• Jump back to A.cc file.
  
  I would be very happy if A.cc can see the unsaved A.h when I am editing A.cc. I think the update should be relatively less frequent with this approach, as people don't usually update multiple files at the same time. Not sure if I want all files that depend on A.h to be updated when I edit A.h though; it seems much more complicated and less important (to me).

In D54077#1287282, @LutsenkoDanil wrote:

@klimek If behavior will be configurable, is it ok for you?

In D54077#1287040, @ilya-biryukov wrote:

Thanks for the patch! I believe many people I talked to want this behavior (myself included).
Some people like what we do now more. It feels like it depends on the workflow: for people who auto-save *all* files before build (some editors do it automatically?) the new behavior is the right one, for people who only save current file and rerun build on the console the old behavior is the correct one.
It all boils down to the argument "I want to see same errors that running the compiler would produce".

@klimek, @arphaman, @simark, WDYT?

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

LG

LG

lg

Sorry that I lost track of that, but feel free to ping once / week - unfortunately the patch doesn't apply cleanly, can you rebase?

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.

Yea, if we go down this route I'd go with this by default:

some ? thing :
else ? otherthing :
unrelated ? but :
    finally;

Usually we use match(anyOf(node), hasDescendant(node)). Or did I misunderstand what you want?

lg

In D28462#1148732, @enyquist wrote:

@klimek having gotten that out of the way, I do occasionally drink too much and have sudden urges to re-implement things from scratch. Close it if you need to, since I can't commit to anything, but.... it it happens to be still open on one of those nights, who knows, maybe I'll end up doing it :)

In D28462#1147019, @enyquist wrote:

@klimek fair point. To be honest, I've pretty much lost interest / momentum on this feature, I very much doubt I will ever go back and re-implement from scratch as you suggest.
Not meaning to sound rude, I just don't want to waste anyone's time who is waiting for this (seems there are a few).

In D44609#1143895, @Wawha wrote:

Hello,

after my last modification (require by previous comment), I do not see any feedback or validation for this patch.
Is their something special to do in order to go forward on this patch?

Lambda are more an more used in modern C++, and it's very annoying to not have a way to format them in allman style.

LG.

lg, thx

lg, thanks!

Florian: did you manually add Danny on this (the account is disabled and I'm trying to figure out where / how folks are added).

LG.

In D46024#1129350, @hans wrote:

In D46024#1121242, @rkirsling wrote:

FWIW, please note that this space-before-brace style is not specific to WebKit; CppCoreGuidelines exhibits it as well:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax

This and WebKit's style seem like compelling arguments to support this option.

klimek, djasper: Do you have any objections against landing this?

LG

I believe this was accepted by rnk - are you waiting for specific further feedback?

Is this written down somewhere? https://webkit.org/code-style-guidelines/ doesn't seem to mention it.

LG

In D45726#1071925, @krasimir wrote:

Another point: for the example in the summary about bailing-out early, is there a test for this already? If not, we should add one.

Address comments.

lg

lg

Just from a formatting point of view, why not:

//.   Comment
#.    define X

In D37813#958116, @Typz wrote:

OK.

So you mean a solution like the one discussed earlier would be the way to go?

I mean that we can configure macros in the format style, like "define A(X) class X {". I'm not 100% sure whether we would just try to use the Preprocessor for this, or whether we'd want to only allow a small subset of actual macros, but the general idea would be the same: The UnwrappedLineParser would parse the macro at the expansion location A(X) into an unwrapped line, and then parse the expansion into a child line, with the tokens tha tare not in the argument of the call being marked as fixed (parent child might also be better inverted).

(As a side-note, I want to stress out that we would actually need a 'reversible' description to support the namespace case, to allow generating the end-of-namespace comment)

I've talked with Daniel and we both believe this patch is not the right way to go forward for clang-format. It is adding configuration mechanisms that we need to maintain going forward, without addressing a general problem; specifically for how to handle macros, I believe there is a clear better way to do it that is also fundamentally more useful than adding extra knobs for every problem we encounter. Finally, these changes do not address a fundamental bug, and workarounds exist, so there is no large benefit to do this now in a non-principled way rather than wait for a more principled fix. Overall, I do understand that this is a trade-off, but I hope you understand that we sometimes have to make decisions.

lg

lg

lg

In D37813#945125, @Typz wrote:

I don't think this is really relevant for this tool: if someone changes the implementation of the macro, then *they* must indeed if it should not be formatted like a namespace (and keep the clang-format configuration unchanged), or if it should now be formatted like a class (and thus make changes to clang-format configuration). Here we are not defining what the macro does, but how clang-format should indent it : in most case I don't think the indentation format should actually depend on the way it is implemented...

In D37813#942137, @Typz wrote:

As far as "parsing" and formatting inside the block is concerned, this is indeed unrelated (and would totally work if all macros where specified with some preprocessor definitions).

But identifying the 'opening' token and generating the matching 'closing' comment are totally related; it would seem very strange to have an option to specify that TESTSUITE() macros are parsed as namespace, then another option to indicate that namespace declared by this macros are actually closed with another macro call...

In D33589#942128, @Typz wrote:

Indeed, looks good, thanks!

Though that exacerbates the alignment issue, I now get things like this:

enum {
  SomeLongerEnum, // comment
  SomeThing,      // comment
  Foo, // something
} 
                            ^ (column limit)

The comment on 'Foo' would overflow a bit, but it gets unindented during "alingment" stage, which kind of 'breaks' the decision that was made earlier on *not* to break the comment...

In D37813#941987, @Typz wrote:

Definitely that would be much more expressive. But this approach is also incomplete: in case of namespace (and possibly others?), we also need to perform the reverse operation, e.g. to "generate" a macro call for rewriting the closing comment.

On top of this, I think that is really not trivial, I would prefer to move forward with these "simpler" patch, and change the whole macro configurations part in later commits...

In D33589#941979, @Typz wrote:

I think the difference between code and comments is that code "words" are easily 10 characters or more, whereas actual words (in comments) are very often less than 10 characters: so code overflowing by 10 characters is not very frequent. whereas small words in comment will often get closer to the "extra" limit.

That said, I tried with your latest change ("Restructure how we break tokens", sha1:64d42a2fb85ece5987111ffb908c6bc7f7431dd4). and it's working about fine now. For the most part it seems to wrap when I would expect it, great work!
I have seen 2 "issues" though:

• Often I see that the last word before reflowing is not wrapped (eventhough it overlaps the line length); I did not count penalties so I cannot confirm this is really an issue or just a borderline scenario.
• Alignment seems better than before, but since there is no penalty for breaking alignment it will always try to unindent to compensate for overflowing characters...
  
  Seeing this, I guess this patch does not make much sense anymore, I'll see if I make some improvements for these two issues, in separate patches.

Add test.

Fix incorrect return value leading to unnecessary computation.