[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

Wed Jan 9 07:46:19 PST 2019

steveire added a comment.

In D56444#1351194 <https://reviews.llvm.org/D56444#1351194>, @JonasToth wrote:

> >> Is the suggestion to make that change, but then modify the semantics of the `functionDecl()` etc matchers to hide it? Or something else?
> > 
> > My suggestion is to extract the traverser from ASTDumper first, then fix this issue.
>
> I understand that you are working on a clean solution which is fine and can be done in 9.0, but:
>
> - this patch is small, almost non-intrusive and can be obsolete in 9.0, i think no one would be sad about that
> - we need to fix this issue, lambda traversal is absolutly normal in ASTMatchers. Once you include the STL in your code, you automatically encounter this use case
> - we break current code that is in trunk already (for a longer time)
> - this patch does not break any other tests so far, so it does not seem to inccur bigger issues, on the other hand redoing AST matching most likely will.
>
>   The semantic issues that came up make sense to address (within this or a similar complex patch), but the current change does not seem to change a lot. Even if it does, introducing false positives/negatives for lambda-cases is still better then leaving the crashing as is.

Yes, I was mostly responding to the suggestion to treat lambdas as functionDecls(), which I think is too drastic to do now, with other solutions on the horizon which may be better.

I don't object to something targeted to fixing the assert as you describe and which this patch seems to be. Sorry if that wasn't clear.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56444/new/

https://reviews.llvm.org/D56444