Szelethus marked 3 inline comments as done.
Szelethus added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1609-1613
+ if (B->rbegin()->getKind() != CFGElement::Kind::Statement)
+ return nullptr;
+
+ // This should be the condition of the terminator block.
+ const Stmt *S = B->rbegin()->castAs<CFGStmt>().getStmt();
----------------
NoQ wrote:
> A bit clearner:
>
> ```lang=c++
> auto StmtElem = B->rbegin().getAs<CFGStmt>();
> if (!StmtElem)
> return nullptr;
>
> const Stmt *S = StmtElem->getStmt();
> ```
>
> Also how about `CFGBlock::getTerminatorCondition()`?
I peeked at it's implementation, and it seems to be incorrect.
Referencing @xazax.hun's inline:
> I vaguely recall some problem with the results of getTerminatorStmt for
> logical operators like &&. I believe what we really want is the last
> expression we evaluated in the basic block which will be the last Stmt of the
> basic block. So if we can settle with the last stmt we can get rid of this
> code.
And this is what `CFGBlock::getTerminatorCondition()` does:
```lang=c++
Stmt *Terminator = getTerminatorStmt();
if (!Terminator)
return nullptr;
Expr *E = nullptr;
switch (Terminator->getStmtClass()) {
// ...
case Stmt::BinaryOperatorClass: // '&&' and '||'
E = cast<BinaryOperator>(Terminator)->getLHS();
// ....
}
return E;
```
But nevertheless, maybe it'd be good to fix this in there and use it here.
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1640-1642
+ CFGBlock *OriginB = GetRelevantBlock(Origin);
+ if (!OriginB || !NB)
+ return nullptr;
----------------
NoQ wrote:
> `// TODO: This can be cached.`
Like, caching the `CFGBlock`s for given `ExplodedNode`s?
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1646
+ if (const Expr *Condition = getTerminatorCondition(NB))
+ if (BR.addTrackedCondition(Condition))
+ bugreporter::trackExpressionValue(
----------------
NoQ wrote:
> All right, i still don't understand this caching based on condition
> expression.
>
> You mean, like, if we're encountering the same condition multiple times (say,
> in a loop), we should only track it once? Why? Like, its values (which are
> the thing we'll really be tracking) may be different (say, on different loop
> iterations).
Yup, can't argue with that, I'll revise this.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62883/new/
https://reviews.llvm.org/D62883
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
