[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

Mon Aug 31 01:26:12 PDT 2020

Szelethus added a comment.
Herald added a subscriber: danielkiss.

In D86736#2244365 <https://reviews.llvm.org/D86736#2244365>, @martong wrote:

>> For any other loops, in order to know whether we should analyze another iteration, among other things, we evaluate it's condition. Which is a problem for ObjCForCollectionStmt, because it simply doesn't have one
>
> Shouldn't we try to fix the ObjCForCollectionStmt in the AST rather? By adding the condition to that as a (sub)expression? There are already many discrepancies and inconsistencies between supposedly similar AST nodes, and people do fix them occasionally (e.g. D81787 <https://reviews.llvm.org/D81787>).

To be fair, before I say anything, I don't know much about Objective C, but I don't think this is a bug. This is just how for each loops are in it. The C++ standard specifies <https://eel.is/c++draft/stmt.ranged#1> that ranged based for loops must be equivalent to a pre-C++11 loop, hence the implicit condition in the AST. I think this is the same kind of annoyance we chatted about regarding virtuality in the AST -- even if a method is virtual, it may not show up as such in the AST if the keyword itself is absent.

> To be honest, it seems a bit off to store some parts of the liveness info in the GDM while other parts not in the GDM ...

There is no liveness to talk about here, it just simply doesn't make sense. This is just a property: "Does this loop have any more iterations left?". While the earlier hack looks convincing that this has something more to it, it really doesn't. In fact, this patch demonstrates it quite well: we only need to keep track of this information until we make a state split, so theoretically, we could just pass this along as a parameter. The only thing holding me back from that solution is that it would be a lot more disruptive change, and would require a change to the `checkBranchCondition` callback.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86736