[PATCH] D67382: [analyzer] NFC: Move getStmt() and createEndOfPath() out of PathDiagnostic.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM! I think code readability improved geatly.

> This creates a certain problem in `RetainCountChecker` (surprise!!~)

We constantly bully this checker, but still not enough :^)



================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h:284
+  getValidSourceLocation(const Stmt *S, LocationOrAnalysisDeclContext LAC,
+                         bool UseEnd = false);
+
----------------
Lets explain what `UseEnd` is: `Whether to use getEndLoc() rather then getBeginLoc() for \p S`.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h:276
+
+  /// Find the next statement that is going to be executed after this node.
+  /// Useful for explaining control flow that follows the current node.
----------------
Is this correct? Shouldn't we instead say that "Find the next statement that was symbolically executed on this path of execution"?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h:282
+
+  /// Find the statement that was executed immediately before that node.
+  /// Useful when the node corresponds to a CFG block entrance.
----------------
before this node?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h:288
+
+  /// Find the statement that was executed at or immediately before that node.
+  /// Useful when any nearby statement will do.
----------------
before this node?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2185
   } else {
-    assert(ErrorNode);
-    hash.AddPointer(GetCurrentOrPreviousStmt(ErrorNode));
----------------
Why delete the assert?


================
Comment at: clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp:302-303
   // Find the node's current statement in the CFG.
-  if (const Stmt *S = PathDiagnosticLocation::getStmt(this))
+  // FIXME: getStmtForDiagnostics() does nasty things in order to provide
+  // a valid statement for body farms, do we need this behavior here?
+  if (const Stmt *S = getStmtForDiagnostics())
----------------
But still fails...


================
Comment at: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp:565-566
 
   // FIXME: Ironically, this assert actually fails in some cases.
   //assert(L.isValid());
   return L;
----------------
I guess didn't change much :^)


Repository:
  rC Clang

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

https://reviews.llvm.org/D67382