xazax.hun requested changes to this revision. xazax.hun added inline comments. This revision now requires changes to proceed.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:176 + const ExplodedNode *Origin; + CFGControlDependencyTree ControlDepTree; + llvm::SmallSet<const CFGBlock *, 32> VisitedBlocks; ---------------- I think we should make clear that this visitor only operates within one function and does not track controll dependencies across functions. ================ Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:680 +} // end of anonymous namespace + ---------------- What was the reason of adding these begin/ends? I prefer to keep such refactorings separate, also it might be controversial whether this change is desired. ================ Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1863-1864 + + case Stmt::ObjCForCollectionStmtClass: + return cast<ObjCForCollectionStmt>(S)->getCollection(); + ---------------- NoQ wrote: > Szelethus wrote: > > @NoQ Any feelings on this? > This makes some sense in the long run but i think you should give up here for > now. Unlike `CXXForRangeStmt`, its Objective-C counterpart doesn't mock up > the AST that would have made it look like a regular for-loop, so there just > isn't an AST node that would represent the part of it that corresponds to > "`__begin != __end`". > > Even in the C++ case, i'm not sure your current behavior would make much > sense. We should probably delegate this work to a checker that knows how > collections work and what makes them empty or have a certain size, something > similar to what the `IteratorChecker` seems to be becoming :) > > Should we give mark the report as invalid when we give up here? I've no idea, > i guess i'll have to gather more empirical evidence on that. 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. ================ Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1899-1901 + CFGBlock *NB = GetRelevantBlock(N); + if (!VisitedBlocks.insert(NB).second) + return nullptr; ---------------- NoQ wrote: > I didn't really understand this part. I guess detecting control depdendency only makes sense for the last statement of the basic block (which is the first one we see in the visitor order). But instead of maintaining a state with the visited nodes we could also check if the statement is the last one of its basic block. Repository: rC Clang 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
