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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/Sema/AnalysisBasedWarnings.h:101
+  void RegisterVarDeclWarning(VarDecl *VD, PossiblyUnreachableDiag
+                       PossiblyUnreachableDiag);
+
----------------
`git-clang-format HEAD~<number of commits>`

The formal parameter should be abbreviated, maybe `PUD`?


================
Comment at: clang/include/clang/Sema/AnalysisBasedWarnings.h:110
 
+void emitPossiblyUnreachableDiags(Sema &S, AnalysisDeclContext &AC,
+SmallVector<PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags);
----------------
How about making this a proper method of `AnalysisBasedWarnings` rather than a free floating function that's only ever called from other methods of `AnalysisBasedWarnings`?  That way you don't have to pass in a `Sema`.


================
Comment at: clang/lib/Analysis/AnalysisDeclContext.cpp:124
+    if(VD->hasGlobalStorage()) {
+      return const_cast<Stmt*>(dyn_cast<Stmt>(VD->getInit()));
+    }
----------------
The `const_cast` doesn't look necessary here.  Is it?


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2012
 
+void clang::sema::emitPossiblyUnreachableDiags(Sema &S, AnalysisDeclContext &AC,
+SmallVector<clang::sema::PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags) {
----------------
having the full namespace spelled out here in the definition smells funny.  Is there a pair of `namespace` blocks below for `clang` and `sema` where the definition of `emitPossiblyUnreachableDiags` could be moved into?

Actually looking at the file, I see you simply matched the existing style, but line 49 currently has a `using` statement that should inject the `clang` namespace into the current scope.  That's why you see `sema::` used in other places in this translation unit without the `clang` namespace prefix.  The whole file should remove the `clang::` prefixes or additionally replace the `using` statement with explicit `namespace` blocks.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2260
+void clang::sema::AnalysisBasedWarnings::RegisterVarDeclWarning(VarDecl *VD,
+    clang::sema::PossiblyUnreachableDiag PossiblyUnreachableDiag) {
+  VarDeclPossiblyUnreachableDiags[VD].emplace_back(PossiblyUnreachableDiag);
----------------
`PUD`


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2275
+
+  clang::sema::emitPossiblyUnreachableDiags(S, AC, VarDeclPossiblyUnreachableDiags[VD]);
+}
----------------
If you make `emitPossiblyUnreachableDiags` then it doesn't need all the prefixes.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:347
+  // context before the function scope or diagnostics are
+  // delayed until function scope is added
+  DeclContext *Cur = CurContext;
----------------
Use punctuation in your comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16678
+    // Mangling context seems to only be defined on constexpr vardecl that
+    // displayed errors
+    // This skips warnings that were already emitted as notes on errors.
----------------
Punctuation.


================
Comment at: clang/test/Sema/warn-unreachable-warning-var-decl.cpp:39-40
+
+constexpr signed char c1 = 100 * 2; // ok expected-warning{{changes value}}
+constexpr signed char c2 = '\x64' * '\2'; // also ok  expected-warning{{changes value}}
+constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}
----------------
`ok` and `also ok` can probably be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63889