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

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

steakhal added a comment.

Fixed remarks, cleaned up residual includes, rebased.
Should be read to land.
Final thoughts?



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:501
   public:
+    const iterator &operator*() const { return *this; }
+
----------------
steakhal wrote:
> 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.
Added comments to both places.


================
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;
----------------
steakhal wrote:
> 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.
Now I remember.
The `referenced_vars` is implemented as `llvm::make_range(referenced_vars_begin(), referenced_vars_end())`, thus I need those functions as they do quite a bit of work if you check.
So, I kept 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 {
----------------
steakhal wrote:
> 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.
This was just an oversight. Removed. Thanks!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp:72
 
-    for (auto B=CondS->symbol_begin(), E=CondS->symbol_end(); B != E; ++B) {
-      const SymbolRef Antecedent = *B;
+    for (SymbolRef Antecedent : CondS->symbols()) {
       State = addImplication(Antecedent, State, true);
----------------
Oh, I didn't know this is a word :D


================
Comment at: clang/lib/StaticAnalyzer/Core/Environment.cpp:29
 #include "llvm/ADT/ImmutableMap.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
----------------
Removed this extra include.


================
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);
+        }
----------------
steakhal wrote:
> 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.
Clang-format went crazy, and I'm using clang-format on save, thus messed this up.
Fixed up manually. Should be good now.


================
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) {
----------------
steakhal wrote:
> xazax.hun wrote:
> > Ha about using `capture_inits`?
> I would count this as a "fancy" iteration as we increment/advance more logical sequences together.
I had a better idea and llvm::zip all these together.
Check it out, and tell me if you like it more ;)
I certainly do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154325