NoQ added inline comments.
================ 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: > baloghadamsoftware wrote: > > 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. > I am still not sure I like this picture. Looking at [B1] I had the impression > it has 3 successors which is clearly not the case. *confirms that 3 successors is 1 successor too many* ================ Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1753-1755 + // It so happens that CFGBlock::getTerminatorCondition returns 'A' for block + // B1, 'A && B' for B2, and 'A && B || C' for B3. Let's check whether we + // reached the end of the condition! ---------------- Szelethus wrote: > NoQ wrote: > > Szelethus wrote: > > > NoQ wrote: > > > > Clever trick, but why not match for logical operators directly? > > > > Something like this: > > > > ```lang=c++ > > > > if (auto B = dyn_cast<BinaryOperator>(OuterCond)) > > > > if (B->isLogicalOp()) > > > > return isAssertlikeBlock(Else, Context); > > > > ``` > > > What about `assert(a + b && "Shouldn't be zero!");`? > > Mmm, could you elaborate? >.< > ```lang=c++ > if (auto B = dyn_cast<BinaryOperator>(OuterCond)) > if (B->isLogicalOp()) > return isAssertlikeBlock(Else, Context); > ``` > We don't need `isLogicalOp()` here. > > Also, I should probably have a testcase for the elvis operator (`?:`). Mmm, i'm still confused. The `a + b` in your example doesn't appear as an else-block terminator anywhere. And i don't see why would `a + b` behave differently compared to a simple `a`, while i do see why would, say, `a && b` behave differently. ================ Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:469-471 +extern void __assert_fail (__const char *__assertion, __const char *__file, + unsigned int __line, __const char *__function) +__attribute__ ((__noreturn__)); ---------------- Szelethus wrote: > Szelethus wrote: > > NoQ wrote: > > > Szelethus wrote: > > > > NoQ wrote: > > > > > I'm pretty sure you can define this function only once. > > > > Note that it is in a namespace! > > > I mean, why is it in a namespace? Why not just define it once for the > > > whole file? It's not like you're gonna be trying out different variants > > > of `__assert_fail`. > > Yea, good point. > Actually, I kinda like how a single namespace is a single test case, an > all-in-one package. Do you insist? Mmm, i guess it's more about `__assert_fail` never actually residing in a namespace in real life (being a C function at best). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65287/new/ https://reviews.llvm.org/D65287 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
