[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer

[Balázs Benics via Phabricator via cfe-commits]

steakhal planned changes to this revision.
steakhal marked an inline comment as done.
steakhal added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:501
   public:
+    const iterator &operator*() const { return *this; }
+
----------------
xazax.hun wrote:
> Is this just a dummy to make sure this satisfies some concepts? I think I comment might be helpful in that case, people might find a noop dereference operator confusing. Same for some other places.
Yes. I'll add a comment.
This falls into the "weird" iterators category.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:749-750
 
   referenced_vars_iterator referenced_vars_begin() const;
   referenced_vars_iterator referenced_vars_end() const;
+  llvm::iterator_range<referenced_vars_iterator> referenced_vars() const;
----------------
xazax.hun wrote:
> Do we need these with the addition of `referenced_vars`?
I no longer remember where, but yes. But for sure I went through these to see if I could remove them.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:637-638
 
   region_iterator region_begin() const { return LiveRegionRoots.begin(); }
   region_iterator region_end() const { return LiveRegionRoots.end(); }
+  llvm::iterator_range<region_iterator> regions() const {
----------------
xazax.hun wrote:
> Should we remove these?
I'm pretty sure we need these somewhere.
One prominent place is where we dump these into JSONs. We do some fancy iteration there. I'm not sure that is the reason for this particular case.
I'll give it another look to confirm.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp:194-195
 
-      for (CallExpr::const_arg_iterator ai = i->AllocCall->arg_begin(),
-           ae = i->AllocCall->arg_end(); ai != ae; ++ai) {
-        if (!(*ai)->getType()->isIntegralOrUnscopedEnumerationType())
+      for (const Expr *Arg : make_range(CallRec.AllocCall->arg_begin(),
+                                        CallRec.AllocCall->arg_end())) {
+        if (!Arg->getType()->isIntegralOrUnscopedEnumerationType())
----------------
xazax.hun wrote:
> 
Makes sense.


================
Comment at: clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp:54
+  for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+    delete Consumer;
   }
----------------
xazax.hun wrote:
> Hm, I wonder whether we actually want to use `unique_ptr`s to make the ownership explicit. Feel free to leave as is in this PR.
The ownership model of these consumers is so messed up.
It's not that simple to fix. I was bitten by dangling consumers/graphs so many times now only counting **last** year.
So, yea. Maybe one day.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:173-177
+        } else {
         // The left-hand side may bind to a different value then the
         // computation type.
         LHSVal = svalBuilder.evalCast(Result, LTy, CTy);
+        }
----------------
xazax.hun wrote:
> Might be an issue with Phab, but indent looks strange to me here.
Thanks for catching this. Indeed. I'll have a look. Probably I messed something up.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1190
   CXXRecordDecl::field_iterator CurField = LE->getLambdaClass()->field_begin();
-  for (LambdaExpr::const_capture_init_iterator i = LE->capture_init_begin(),
-                                               e = LE->capture_init_end();
-       i != e; ++i, ++CurField, ++Idx) {
+  for (auto i = LE->capture_init_begin(), e = LE->capture_init_end(); i != e;
+       ++i, ++CurField, ++Idx) {
----------------
xazax.hun wrote:
> Ha about using `capture_inits`?
I would count this as a "fancy" iteration as we increment/advance more logical sequences together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154325