martong added a comment. In D86736#2247035 <https://reviews.llvm.org/D86736#2247035>, @Szelethus wrote:
> 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. Ok, fair enough. ================ Comment at: clang/lib/StaticAnalyzer/Core/ProgramState.cpp:327 +using ObjCForLctxPair = + std::pair<const ObjCForCollectionStmt *, const LocationContext *>; + ---------------- Why it is not enough to simply have ObjCForCollectionStmt* as a key? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86736/new/ https://reviews.llvm.org/D86736 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
