[PATCH] D63538: [CFG] Add a new function to get the proper condition of a CFGBlock

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/Analysis/CFG.cpp:5619-5625
+  // If the terminator is a temporary dtor or a virtual base, etc, we can't
+  // retrieve a meaningful condition, bail out.
+  if (rbegin()->getKind() != CFGElement::Kind::Statement)
+    return nullptr;
+
+  // This should be the condition of the terminator block.
+  const Stmt *S = rbegin()->castAs<CFGStmt>().getStmt();
----------------
NoQ wrote:
> I don't think you're looking at the terminator here, as `CFGTerminator` isn't a sub-class of `CFGElement`. So this if doesn't avoid temporary dtor branches or virtual base branches; instead, it avoids various other `CFGElement` sub-classes such as `CFGInitializer` or `CFGImplicitDtor` (not sure how many of those may appear last in a block).
> 
> Therefore the following code would be a bit more LLVM-y:
> ```lang=c++
> auto StmtElem = rbegin()->getAs<CFGStmt>();
> if (!StmtElem)
>   return nullptr;
> 
> const Stmt *S = StmtElem->getStmt();
> ```
> 
> Also, are you sure that the last expression is always a condition? Like, what about
> ```lang=c++
> void foo(int x, int y) {
>   (void)(x + y);
> }
> ```
> ?
The question is, is `CFGTerminator ` is what we are looking for? I think the point of the method is to get the last subexpression that was evaluated before taking a branch. 

For the condition, would it make sense to check the number of successors (before pruning the trivially false branches)? If less than or equal to 1 it is probably not a condition. Otherwise maybe we could just rename it to last evaluated (sub)expression. The sub-part might depend on whether we use a linearized CFG.


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

https://reviews.llvm.org/D63538