[PATCH] D146342: [-Wunsafe-buffer-usage] Move the whole analysis to the end of a translation unit

[Ziqing Luo via Phabricator via cfe-commits]

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