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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
