[PATCH] D63889: Check possible warnings on global initializers for reachability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Parse/Parser.h:1873
+  ExprResult ParseInitializer(Decl *DeclForInitializer = nullptr) {
+    Actions.ExprEvalContexts.back().DeclForInitializer = DeclForInitializer;
+    ExprResult init;
----------------
This should be done by calling a function on Sema (add an `ActOnStartDeclInitializer` or similar), not by directly modifying Sema's internal state.


================
Comment at: clang/include/clang/Parse/Parser.h:1880
+    }
+    Actions.ExprEvalContexts.back().DeclForInitializer = nullptr;
+    return init;
----------------
Please add an assertion when you first set this that the old value was null.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2263
+    VarDecl *VD) {
+  AnalysisDeclContext AC(nullptr, VD);
+
----------------
Consider grabbing `VarDeclPossiblyUnreachableDiags[VD]` first and avoiding constructing the `AnalysisDeclContext` at all in the (presumably overwhelmingly common case) that there are no such diagnostics.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11853
 
+  AnalysisWarnings.IssueWarningsForRegisteredVarDecl(var);
+
----------------
We should (presumably) only do this for file-scope variables. The initializers for block-scope variables will be checked when checking the enclosing function (if the variables' declarations are reachable at all).


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4884
 
+  AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
----------------
We should only do this once, when we instantiate the default argument, rather than once each time we use it. (Move this up to just before line 4856?)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16670
   case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
     if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
+      FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
----------------
This call to `getCurFunctionOrMethodDecl()` is wrong; it will skip over lambda-expressions. (This is why you're still seeing diagnostics in file-scope lambdas.) Something like this should work:

```
if (Stmts.empty()) {
  Diag(Loc, PD);
  return true;
}

if (FunctionScopeInfo *FSI = getCurFunction()) {
  FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
      PossiblyUnreachableDiag(PD, Loc, Stmts));
  return true;
}

// [handle VarDecl case]
```


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16680-16682
+      if (VD->getDefinition()) {
+        VD = VD->getDefinition();
+      }
----------------
No braces here please. Also, this appears to be wrong: we want the declaration with the initializer (which is always `VD`), not the definition (which might be a different declaration for a static data member), don't we?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16690
         break;
-      // FIXME: For any other kind of variable, we should build a CFG for its
-      // initializer and check whether the context in question is reachable.
+      // FIXME: Some cases aren't picked up by path analysis currently
+      if (!Stmts.empty() && VD->isFileVarDecl()) {
----------------
What does this fixme mean?


================
Comment at: clang/test/SemaTemplate/instantiate-static-var.cpp:9
   static const T value = 10 / Divisor; // expected-error{{in-class initializer for static data member is not a constant expression}}
+  //expected-warning at -1 {{division by zero is undefined}}
 };
----------------
We should not warn here (because this initializer is required to be a constant expression).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63889