[PATCH] D87527: [ASTMatchers] Fix `hasBody` for the descendants of `FunctionDecl`

Mon Sep 14 06:10:05 PDT 2020

ymandel added a comment.

In D87527#2271016 <https://reviews.llvm.org/D87527#2271016>, @baloghadamsoftware wrote:

> In D87527#2270939 <https://reviews.llvm.org/D87527#2270939>, @ymandel wrote:
>
>> Can you expand on what is wrong currently for `FunctionDecl` descendants? Would the new test `FindsBodyOfFunctionChildren` fail with the current implementation?
>
> Yes, exactly. They return 2 findings instead of 1 because the matcher matches both the forward declaration and the definition. The cause for this is that template specializations do not work for the descendants of the class for which is template specialized. Instead, the generic version is used. Thus for the children of `FunctionDecl` the original `GetBodyMatcher` is instantiated which does not check whether `doesThisDeclarationHaveABody()` but returns a body instead which may be bound to another declaration of the same function in the declaration chain. This is obviously wrong.

Thank you for clarifying. I might nitpick that it's not "obviously wrong": I'm actually writing code now that depends on exactly that behavior -- I don't care where the method decl body is, as long as it's visible in the current TU.  That said, I think you're right in wanting `FunctionDecl`'s behavior to align with that of its derived classes and I think it better to behave that way than how it currently behaves.  But, we should have an alternative available in case this change breaks anyone else depending on the current behavior.  Is there a different matcher with the semantics I've described?

Also, please clarify the comments in the header for this matcher. The current description is quite unclear for functions: "Matches a 'for', 'while', 'do while' statement or a function
definition that has a given body."

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87527