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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2320-2323
+    void traverseTU(const TranslationUnitDecl *TU) {
+      for (auto I = TU->decls_begin(); I != TU->decls_end(); ++I)
+        TraverseDecl(*I);
+    }
----------------
`RecursiveASTVisitor` does this automatically. Just ask it to `Visit(TU)`.


================
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:
----------------
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.


================
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`:
----------------
I wonder if `ObjCMethodDecl` has the same problem. They too can have prototypes and definitions separate from each other.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2363-2364
+      if (isa<LambdaExpr>(Node)) {
+        // to visit implicit children of `LambdaExpr`s:
+        IsVisitingLambda = true;
+
----------------
I suspect that you might have reinvented `shouldVisitLambdaBody()`.


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

https://reviews.llvm.org/D146342