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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1743
+
+  if (Then->isInevitablySinking() || Else->isInevitablySinking())
+    return true;
----------------
xazax.hun wrote:
> Do we consider a block assert like if both branches are inevitable sinking?
No, I think `(Then->isInevitablySinking() != Else->isInevitablySinking())` would serve better here.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+  //                       [B1] -> [B2] -> [B3] -> [sink]
+  // assert(A && B || C);    \       \       \
+  //                          -------------------> [go on with the execution]
----------------
xazax.hun wrote:
> 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 :)
I think an arrow from `[B1]` to `[B3]` is missing for the `A == false` case.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1760
+
+    using namespace ast_matchers;
+    auto IsSubStmtOfCondition =
----------------
I would avoid using matchers in the core infrastructure. My experience says they are quite expensive to create and destroy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65287