Szelethus marked 2 inline comments as done.
Szelethus added a comment.
After testing this patch out, I'm confident it works pretty well. Take a look
at the following two runs: 138 notes
<http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=27f8f44ceff3132c3a2232bb760804d8&report=7893&subtab=27f8f44ceff3132c3a2232bb760804d8>
-> 20 notes
<http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=27f8f44ceff3132c3a2232bb760804d8&report=13010&subtab=27f8f44ceff3132c3a2232bb760804d8>.
================
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:
> > > 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 (`?:`).
================
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:
> > 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.
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
