Szelethus added inline comments.
================
Comment at: clang/lib/Analysis/CFG.cpp:5634
+ // immediately caught.
+ if (std::any_of(Blk->begin(), Blk->end(), [](const CFGElement &Elm) {
+ if (Optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
----------------
xazax.hun wrote:
> I am wondering, should we actually scan the whole `CFGBlock` for
> `ThrowExpr`s? Shouldn't `Throw` be the terminator? (In case we can get that
> easily) In case we are afraid of seeing lifetime end or other blocks AFTER
> throw, maybe it is more efficient to start scanning from the last element ad
> early return on the first non-throw stmt?
I just moved this code around, I'd be happy to tinker with this later maybe in
a followup patch. To confidently make a change like this would require me to
write a bunch more tests and study how the block is built, and I'm not even
terribly familiar with how exceptions work -- would you mind if I delayed this?
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+ // [B1] -> [B2] -> [B3] -> [sink]
+ // assert(A && B || C); \ \ \
+ // -------------------> [go on with the execution]
----------------
NoQ wrote:
> 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*
Alright. I think I got it now (eventually).
================
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!
----------------
NoQ wrote:
> 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.
Right. I take it all back. But I guess we might as well just `assert` that it's
a logical operator, if it has 2 successors.
================
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__));
----------------
NoQ wrote:
> 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).
I could actually just use `[[noreturn]] void halt();`, but this looks cooler. :)
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
