[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/CFG.h:860
+  /// child CFG blocks in a depth-first manner and see if all paths eventually
+  /// end up in an immediate sink block.
+  bool isInevitablySinking() const;
----------------
Is it obvious what sink means in CFG's context? Maybe we should mention what our definition of sink is? Think about non-csa devs too :)


================
Comment at: clang/lib/Analysis/CFG.cpp:5628
+
+  // FIXME: Throw-expressions are currently generating sinks during analysis:
+  // they're not supported yet, and also often used for actually terminating
----------------
Are there other nodes that are treated as sinks? If so I wonder if there is a way to keep the list updated. Oh, I just saw that this code is just moved around. 


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1743
+
+  if (Then->isInevitablySinking() || Else->isInevitablySinking())
+    return true;
----------------
Do we consider a block assert like if both branches are inevitable sinking?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+  //                       [B1] -> [B2] -> [B3] -> [sink]
+  // assert(A && B || C);    \       \       \
+  //                          -------------------> [go on with the execution]
----------------
I wonder if the CFG is correct for your example. If A evaluates to false, we still check C before the sink. If A evaluates to true we still check B before going on. But maybe it is just late for me :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65287