ziqingluo-90 added inline comments.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2335
+ // To analyze callables:
+ if (isa<FunctionDecl, BlockDecl, ObjCMethodDecl>(Node)) {
+ // For code in dependent contexts, we'll do this at instantiation time:
----------------
NoQ wrote:
> The intended way to make recursive visitors is to have
> `TraverseFunctionDecl`, `TraverseBlockDecl`, etc., which automatically do the
> `isa` check and fall through to the superclass if the method isn't defined.
> Then they can all call a common method to analyze the body.
>
> This doesn't necessarily lead to better code though, just to more traditional
> code, so up to you^^ in this case it probably doesn't matter. But it's
> definitely nice to separate the common part (the part that invokes all
> individual analyses) into its own method. And then you can eg. isolate the
> special `hasBody()` check in the `TraverseFunctionDecl()` method, so that
> other code paths didn't have an extra runtime check.
yeah, I think overriding `VisitFunctionDecl` and others separately is a better
form because I was clearly dealing with `FunctionDecl` and others case by case
there. So put the specific logic for `FunctionDecl` in `VisitFunctionDecl`
makes it clear.
Thanks for your advice.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2343-2345
+ // `FunctionDecl->hasBody()` returns true if the function has a body
+ // somewhere defined. But we want to know if this `Node` has a body
+ // child. So we use `doesThisDeclarationHaveABody`:
----------------
NoQ wrote:
> I wonder if `ObjCMethodDecl` has the same problem. They too can have
> prototypes and definitions separate from each other.
It looks like `hasBody()` works properly for `ObjCMethodDecl`. I'm adding a
test for it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146342/new/
https://reviews.llvm.org/D146342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
