LGTM!

LGTM aside from some nits, thank you for this!

In D61508#1509368, @trixirt wrote:

Latest change addresses most of issues.
Outstanding is adding a test when garbage GuardStyle value is fed into config.
The trial testcase used only CHECK-*-NOT which caused complaining from test harness looking for the CHECK- cases.
Looking for something similar in the other checker tests did not turn up anything.
In general it seems like clang-tidy assumes developers get the string names of the options and their values correct and does not provide a lot of (any?) checking.

If you wanted to expand test coverage for the changes covered in this, that would be a good thing. However, I'm not certain how testable the newly colorized output is, which is why this LG as-is.

LGTM

LGTM (with the typo fix).

In D61834#1505418, @steveire wrote:

In D61834#1505124, @aaron.ballman wrote:

In D61834#1505056, @steveire wrote:

In D61834#1504665, @aaron.ballman wrote:

What will be making use of/testing this new functionality?

Any code which has a DynTypedNode and wishes to traverse it.

I envisage this as a more-flexible DynTypedNode::dump that the user does not have to implement themselves in order to use the ASTNodeTraverser.

Do we currently have any such code that's using this functionality, though? I'm mostly concerned that this is dead code with no testing, currently. The functionality itself seems reasonable enough and the code looks correct enough, so if this is part of a series of planned changes, that'd be good information to have for the review.

Ah, yes. This is supposed to be 'useful public API' like the other Visit methods for use inside and outside the codebase. A follow-up patch will use it, but it's provided for external use too anyway.

I'll add a unit test.

In D61835#1505388, @steveire wrote:

In D61835#1505314, @aaron.ballman wrote:

In D61835#1505280, @steveire wrote:

In D61835#1505228, @aaron.ballman wrote:

In D61835#1505202, @steveire wrote:

3. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient?

It doesn't have the same behavior as ASTDumper, because ASTDumper "overrides" some Visit metthods.

I'm aware that they're different, but I may not have been sufficiently clear. I'm saying that the only public APIs I think a user should be calling are inherited from ASTNodeTraverser and not ASTDumper, so it is not required to expose ASTDumper directly, only a way to get an ASTNodeTraverser reference/pointer to an ASTDumper instance so that you get the correct virtual dispatch. e.g., ASTNodeTraverser *getSomethingThatActsLikeAnASTDumper() { return new ASTDumper; }

Perhaps. One way to implement the 'less noise' version of AST output (ie removing pointer addresses etc http://ce.steveire.com/z/HaCLeO ) is to make it an API on the TextNodeDumper. Then ASTDumper would need a forwarding API for it.

Anyway, your argument also applies to JSONNodeDumper, but you put that in a header file.

In D61835#1505280, @steveire wrote:

In D61835#1505228, @aaron.ballman wrote:

In D61835#1505202, @steveire wrote:

3. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient?

It doesn't have the same behavior as ASTDumper, because ASTDumper "overrides" some Visit metthods.

In D61835#1505202, @steveire wrote:

In D61835#1505151, @aaron.ballman wrote:

In D61835#1505048, @steveire wrote:

The users of the follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs need that 'correctness', but also need the public API from ASTNodeTraverser on the instance. (That patch also extends the public API for users).

I don't see anything in the follow-up patch that actually uses the ASTDumper class though, so I'm still a bit confused.

Maybe my wording was confusing. I'll try again:

1. Anyone who wants traversal in the same way that dumping is done must use an instance of the ASTDumper class. Is that much clear? Does my previous response clarify why that is the case?

In D61835#1505048, @steveire wrote:

In D61835#1504663, @aaron.ballman wrote:

I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?

It might help if I had a better idea of which APIs you thought were ones that would help users (because my only real concern with this change is that the public interface for this class is rather unpleasant).

The reason the ASTDumper class still exists (for the purpose of dumping an AST to stream at least) is that it dumps the {Function,Var,Class}TemplateDecl 'correctly'.

In D61834#1505056, @steveire wrote:

In D61834#1504665, @aaron.ballman wrote:

What will be making use of/testing this new functionality?

Any code which has a DynTypedNode and wishes to traverse it.

I envisage this as a more-flexible DynTypedNode::dump that the user does not have to implement themselves in order to use the ASTNodeTraverser.

LGTM aside from some commenting requests.

Thanks for this -- it looks like really interesting functionality! I've mostly found nits thus far, but did have a question about clang-query support for it.

What will be making use of/testing this new functionality?

I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?

LGTM!

I've commit in r360667, thanks!

Commit in r360622.

In D60910#1495673, @rsmith wrote:

If you're happy with these two conditions, then I have no concerns with this moving forward:

• There is no implied stability for the content or format of the dump between major releases, other than that it be valid JSON; this should be stated explicitly in the documentation. (Compatibility between patch releases seems like something we can work out with the release manager, but I'm inclined to say we should make a best-effort attempt to preserve it.) If people want to build tools on this rather than on one of our stable APIs, they should expect to be broken in some way on every major release.
• There is no requirement for people maintaining the AST (changing or adding AST nodes) to update the dump output for modified AST nodes to show any new information -- unlike the existing -ast-dump, this is not just for debugging, but we should be able to treat it as if it were. Perhaps a better way to put it: there is no requirement that the information in this dump is complete, but the information that is dumped should be correct.
  
  If you want stronger guarantees than that, then we should have a broader discussion to establish some community buy-in.

Switched to using the JSON streaming interface rather than a custom solution
Updated the test cases for the new formatting

Ping x2.

LGTM!

In D24892#1500371, @mgehre wrote:

Do I wait for @alexfh to turn his red into a green, too?

Adding some more AST matcher reviewers to see if there are other options that we've not considered.

In D60543#1497576, @stephanemoore wrote:

I did some digging and I believe there are two approaches that we can take to extend isDerivedFrom to support Objective-C classes.

Option 1: Match on Common Ancestor Declaration Type:
Convert isDerivedFrom to match on the common ancestor declaration type, NamedDecl, and return false for
declaration types other than CXXRecordDecl and ObjCInterfaceDecl.

Advantages:
• Largely works in-place without requiring major changes to matchers built on top of isDerivedFrom.

Disadvantages:
• Allows nonsensical matchers, e.g., functionDecl(isDerivedFrom("X")).

Option 2: Convert to Polymorphic Matcher:
Convert isDerivedFrom to a polymorphic matcher supporting CXXRecordDecl and ObjCInterfaceDecl.

Advantages:
• Restricts usage of isDerivedFrom to CXXRecordDecl and ObjCInterfaceDecl.

Disadvantages:
• Potentially requires many or all matchers using isDerivedFrom to refactor to adapt to the polymorphic matcher interface.

Evaluation
I have been going back and forth as to which approach is superior. Option 1 promotes source stability at the cost of usability while
option 2 seems to promote usability at the cost of source stability. I exported a portrayal of option 1 for consideration as it
required less invasive changes. I can see arguments supporting both approaches.

What do you think? Which of the two approaches do you think we should go with? Is there another approach that I have not thought of?

LGTM with some testing nits.

In D56160#1497473, @bernhardmgruber wrote:

• fixed formatting
• fixed function names in tests
• added -fexceptions to test arguments
• fixed typo in release notes

In D56160#1496594, @aaron.ballman wrote:

In D56160#1496391, @bernhardmgruber wrote:

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

I've commit for you in r360345, thank you for the patch!

In D56160#1496391, @bernhardmgruber wrote:

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

LGTM!

In D56160#1495847, @bernhardmgruber wrote:

@aaron.ballman and @JonasToth: Thank you for the patience and all the feedback! It means a great deal to me to have a patch accepted here!

LGTM

LGTM!

In D52967#1494969, @bmwiedemann wrote:

Can someone please merge this change?

In D59280#1494036, @Manna wrote:

In D59280#1493980, @aaron.ballman wrote:

Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.

Thanks Aaron. I don't think I have commit right. Could you please submit the patches?

This LGTM, but you may want to wait a day or so to see if @alexfh has any remaining concerns.

Aside from a formatting issue, this LGTM, thank you!

Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.

LGTM!

LGTM modulo the comments from @gribozavr.

LGTM!

Ping.

Updating based on review feedback.

I've commit in r360147, thank you for the patch!

I found a few nits, but generally think this LG. However, I think @rsmith should sign off on it just in case I've misinterpreted something along the way.

In D60507#1491095, @ztamas wrote:

Ping.
Is it good to go or is there anything else I need to include in this patch?

LGTM!

In D61508#1491570, @JonasToth wrote:

Hi trixirt and thanks for the patch!

I would rather like to generalize the llvm check to allow different styles and then alias the general version with different configurations. Introducing this code duplication does not sound like a good idea to me.
The documentation fixes you make can be done in a separate patch to keep things clean.

You should also regenerate the documentation by running clang/docs/tools/dump_ast_matchers.py.

In D61422#1489159, @baloghadamsoftware wrote:

In D61422#1488081, @aaron.ballman wrote:

Out of curiosity, have you run this over any large code bases to see what the false positive and true positive rate is?

Neither false, nor true positives found so far. I ran it on several open-source projects.

LGTM!

You should also update Registry.cpp to add this to clang-query and you should also regenerate the documentation by running clang\docs\tools\dump_ast_matchers.py.

I would still like to see the second RUN line added to the test so we can ensure we don't regress the behavior.

Out of curiosity, have you run this over any large code bases to see what the false positive and true positive rate is?

LGTM!

In D61239#1486692, @JornVernee wrote:

In D61239#1486634, @aaron.ballman wrote:

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

Yeah, my mistake. It was there after all. The output from clang-format looked the same as what I had before, but I think I just didn't catch the newline when copy-pasting the diff the first time :)

LGTM, thank you!

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

In D61288#1486034, @xbolva00 wrote:

I am not familiar with clang-tidy’s codebase and I personally prefer good compiler warnings than dependency on another tool (clang-tidy). I mean the cases when diagnostic check is easy to do in the compiler.

And in semaexpr we have all we need and it is simple to do it.

But if majority of reviewers disagree with compiler warning, I will close this revision.

In D61288#1486006, @xbolva00 wrote:

Some coding guidelines may require switch to have always default label. Even if devs know that default is not reachable, they can add default: abort(); or assert to increase safety (and warning will be silenced).

Yes, it not suitable to be enabled by default, but I still think it is good to have it.

In D61239#1485994, @JornVernee wrote:

• holding of on adding helper method until hearing back.