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

[Pirama Arumuga Nainar via Phabricator via cfe-commits]

pirama added inline comments.


================
Comment at: clang/include/clang/Sema/AnalysisBasedWarnings.h:111
+void emitPossiblyUnreachableDiags(Sema &S, AnalysisDeclContext &AC,
+SmallVector<PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags);
+
----------------
Fix indentation.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2007
+static void flushDiagnostics(Sema &S,
+SmallVector<clang::sema::PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags) {
+  for (const auto &D : PossiblyUnreachableDiags)
----------------
Should `PossiblyUnreachableDiags` be const?

Also, fix indentation.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2015
+
+  if (!PossiblyUnreachableDiags.empty()) {
+    bool analyzed = false;
----------------
How about returning early if `PossiblyUnreachableDiags` here is empty?  That'll avoid putting the entire logic of this function in the `true` branch.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2031
+          CFGReverseBlockReachabilityAnalysis *cra =
+              AC.getCFGReachablityAnalysis();
+          // FIXME: We should be able to assert that block is non-null, but
----------------
`getCFGReachablityAnalysis` has a typo.  It's missing an 'i'.  Consider fixing this in a separate patch.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2051
+
+    if (!analyzed)
+      flushDiagnostics(S, PossiblyUnreachableDiags);
----------------
Rewrite this as the `else` clause for `if (AC.getCFG())`?  In the current structure, it's not immediately clear that `flushDiagnostics` is called iff `AC.getCFG()` fails to return a valid CFG.

Upon further reading, this seems to be refactored from another function below so it probably makes sense to leave it as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63889