Cleanup MemRegion.cpp/MemRegion.h

[ariccio]

Summary

There are lots of (a) unnecessary, and (b) C-style, casts in MemRegion.cpp, and they were giving me a headache. I also noticed and fixed two non-const member functions.

I also did one drive-by fix where a conditional break was inside an unbraced loop. Code like that is always harder to read, and easily breakable.

Not exactly sure who to add here, so please redirect if you know who is? Looks like dcoughlin, aaron.ballman, and dergachev, are good bets, as they're in previous revisions.

[ariccio]

All checks - except the two that normally don't in my environment - pass! Adding reviewers.

[dcoughlin]

This looks fine to me, except please be careful about extra spaces. Thanks for cleaning this up!

Adding Anna Zaks as a reviewer since she is the code owner for the static analyzer.

C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

253

Extra space here.

258

Extra space here.

265

Extra space here.

273

Extra space here.

281

Extra space here.

298

Extra space here.

338

Extra space here.

457

Extra spaces here.

465

Extra spaces here.

481

Extra spaces here.

486

Extra spaces here.

[dcoughlin]

Also adding cfe-commits as a subscriber. (See http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

[ariccio]

Oops. The funny thing about the extra spaces is that I wasn't sure why there were spaces after the casts already, so I figured that I should keep it that way.

I shall fix.

[ariccio]

Removed extra spaces.

[aaron.ballman]

Aside from one minor nit with auto usage, LGTM!

C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

1396

No need for a const here; the correct change is to not use auto (here and the line above), but instead spell out the type explicitly.

[ariccio]

As said in comment, I disagree with the no need for const here.

C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

1396

The const is partly to make sure that the if (NumBlockVars == 0) { line never accidentally becomes if (NumBlockVars = 0) { like it did in CPython:

http://bugs.python.org/issue25844

1396

For archival reasons, I'll copy & paste the relevant diff here (I hate dead links):

--- a/PC/launcher.c
+++ b/PC/launcher.c
@@ -114,7 +114,7 @@ static wchar_t * get_env(wchar_t * key)
     if (result >= BUFSIZE) {
         /* Large environment variable. Accept some leakage */
         wchar_t *buf2 = (wchar_t*)malloc(sizeof(wchar_t) * (result+1));
-        if (buf2 = NULL) {
+        if (buf2 == NULL) {
             error(RC_NO_MEMORY, L"Could not allocate environment buffer");
         }
         GetEnvironmentVariableW(key, buf2, result);

[aaron.ballman]

C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

1396

While this form of bug can certainly crop up, it's still a bridge-too-far for this project, as I understand it our de facto guidelines on this. I am not certain that we want to start sprinkling const onto value types (as opposed to reference and pointer types) at this point. If we do, it should certainly be something handled a bit more consistently and actively than a general clean-up related to unnecessary type casting.

[ariccio]

I will remove that const later tonight.

C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

1396

If we do, it should certainly be something handled a bit more consistently and actively than a general clean-up related to unnecessary type casting.

You've got a good point there. I'll upload a final revision without that const a bit later tonight.

[ariccio]

Removed single const.

[ariccio]

All set.

[aaron.ballman]

LGTM, but the patch does not apply cleanly because it was created using absolute paths. In the future, please generate the patch with relative paths. ;-)

Commit in r259652

[ariccio]

In D16748#343011, @aaron.ballman wrote:

LGTM, but the patch does not apply cleanly because it was created using absolute paths. In the future, please generate the patch with relative paths. ;-)

Commit in r259652

Oops.

[ariccio]

Grr! missed a single note.

C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

enum InvalidationKinds {
     TK_EntireMemSpace = 0x8
 
     // Do not forget to extend StorageTypeForKinds if number of traits exceed 
     // the number of bits StorageTypeForKinds can store.
   };
 
   void setTrait(SymbolRef Sym, InvalidationKinds IK);
   void setTrait(const MemRegion *MR, InvalidationKinds IK);
-  bool hasTrait(SymbolRef Sym, InvalidationKinds IK);
-  bool hasTrait(const MemRegion *MR, InvalidationKinds IK);
+  bool hasTrait(SymbolRef Sym, InvalidationKinds IK) const;
+  bool hasTrait(const MemRegion *MR, InvalidationKinds IK) const;
 };
   
 } // end GR namespace
 
 } // end clang namespace
 
 //===----------------------------------------------------------------------===//
 // Pretty-printing regions.

C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

RegionTy* MemRegionManager::getRegion(const A1 a1) {
 
   llvm::FoldingSetNodeID ID;
   RegionTy::ProfileRegion(ID, a1, superRegion);
   void *InsertPos;
   RegionTy* R = cast_or_null<RegionTy>(Regions.FindNodeOrInsertPos(ID,
                                                                    InsertPos));
 
   if (!R) {
-    R = (RegionTy*) A.Allocate<RegionTy>();
+    R = A.Allocate<RegionTy>();
     new (R) RegionTy(a1, superRegion);
     Regions.InsertNode(R, InsertPos);
   }
 
   return R;
 }
 
 template <typename RegionTy, typename A1>
 RegionTy* MemRegionManager::getSubRegion(const A1 a1,
                                          const MemRegion *superRegion) {
   llvm::FoldingSetNodeID ID;
   RegionTy::ProfileRegion(ID, a1, superRegion);
   void *InsertPos;
   RegionTy* R = cast_or_null<RegionTy>(Regions.FindNodeOrInsertPos(ID,
                                                                    InsertPos));
 
   if (!R) {
-    R = (RegionTy*) A.Allocate<RegionTy>();
+    R = A.Allocate<RegionTy>();
     new (R) RegionTy(a1, superRegion);
     Regions.InsertNode(R, InsertPos);
   }
 
   return R;
 }
 
 template <typename RegionTy, typename A1, typename A2>
 RegionTy* MemRegionManager::getRegion(const A1 a1, const A2 a2) {
 
   const typename MemRegionManagerTrait<RegionTy>::SuperRegionTy *superRegion =
   MemRegionManagerTrait<RegionTy>::getSuperRegion(*this, a1, a2);
 
   llvm::FoldingSetNodeID ID;
   RegionTy::ProfileRegion(ID, a1, a2, superRegion);
   void *InsertPos;
   RegionTy* R = cast_or_null<RegionTy>(Regions.FindNodeOrInsertPos(ID,
                                                                    InsertPos));
 
   if (!R) {
-    R = (RegionTy*) A.Allocate<RegionTy>();
+    R = A.Allocate<RegionTy>();
     new (R) RegionTy(a1, a2, superRegion);
     Regions.InsertNode(R, InsertPos);
   }
 
   return R;
 }
 
 template <typename RegionTy, typename A1, typename A2>
 RegionTy* MemRegionManager::getSubRegion(const A1 a1, const A2 a2,
                                          const MemRegion *superRegion) {
 
   llvm::FoldingSetNodeID ID;
   RegionTy::ProfileRegion(ID, a1, a2, superRegion);
   void *InsertPos;
   RegionTy* R = cast_or_null<RegionTy>(Regions.FindNodeOrInsertPos(ID,
                                                                    InsertPos));
 
   if (!R) {
-    R = (RegionTy*) A.Allocate<RegionTy>();
+    R = A.Allocate<RegionTy>();
     new (R) RegionTy(a1, a2, superRegion);
     Regions.InsertNode(R, InsertPos);
   }
 
   return R;
 }
 
 template <typename RegionTy, typename A1, typename A2, typename A3>
 RegionTy* MemRegionManager::getSubRegion(const A1 a1, const A2 a2, const A3 a3,
                                          const MemRegion *superRegion) {
 
   llvm::FoldingSetNodeID ID;
   RegionTy::ProfileRegion(ID, a1, a2, a3, superRegion);
   void *InsertPos;
   RegionTy* R = cast_or_null<RegionTy>(Regions.FindNodeOrInsertPos(ID,
                                                                    InsertPos));
 
   if (!R) {
-    R = (RegionTy*) A.Allocate<RegionTy>();
+    R = A.Allocate<RegionTy>();
     new (R) RegionTy(a1, a2, a3, superRegion);
     Regions.InsertNode(R, InsertPos);
   }
 
   return R;
 }
 
 //===----------------------------------------------------------------------===//
QualType CXXBaseObjectRegion::getValueType() const {
   return QualType(getDecl()->getTypeForDecl(), 0);
 }
 
 //===----------------------------------------------------------------------===//
 // FoldingSet profiling.
 //===----------------------------------------------------------------------===//
 
 void MemSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger(static_cast<unsigned>(getKind()));
 }
 
 void StackSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger( static_cast<unsigned>(getKind()));
   ID.AddPointer(getStackFrame());
 }
 
 void StaticGlobalSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger( static_cast<unsigned>(getKind()));
   ID.AddPointer(getCodeRegion());
 }
 
 void StringRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
                                  const StringLiteral* Str,
                                  const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) StringRegionKind);
+  ID.AddInteger(static_cast<unsigned>( StringRegionKind));
   ID.AddPointer(Str);
   ID.AddPointer(superRegion);
 }
 
 void ObjCStringRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
                                      const ObjCStringLiteral* Str,
                                      const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) ObjCStringRegionKind);
+  ID.AddInteger(static_cast<unsigned>( ObjCStringRegionKind));
   ID.AddPointer(Str);
   ID.AddPointer(superRegion);
 }
 
 void AllocaRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
                                  const Expr *Ex, unsigned cnt,
                                  const MemRegion *superRegion) {
-  ID.AddInteger((unsigned) AllocaRegionKind);
+  ID.AddInteger(static_cast<unsigned>( AllocaRegionKind));
   ID.AddPointer(Ex);
   ID.AddInteger(cnt);
   ID.AddPointer(superRegion);
 }
 
 void AllocaRegion::Profile(llvm::FoldingSetNodeID& ID) const {
   ProfileRegion(ID, Ex, Cnt, superRegion);
 }
 
 void CompoundLiteralRegion::Profile(llvm::FoldingSetNodeID& ID) const {
   CompoundLiteralRegion::ProfileRegion(ID, CL, superRegion);
 }
 
 void CompoundLiteralRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
                                           const CompoundLiteralExpr *CL,
                                           const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) CompoundLiteralRegionKind);
+  ID.AddInteger(static_cast<unsigned>( CompoundLiteralRegionKind));
   ID.AddPointer(CL);
   ID.AddPointer(superRegion);
 }
 
 void CXXThisRegion::ProfileRegion(llvm::FoldingSetNodeID &ID,
                                   const PointerType *PT,
                                   const MemRegion *sRegion) {
-  ID.AddInteger((unsigned) CXXThisRegionKind);
+  ID.AddInteger(static_cast<unsigned>( CXXThisRegionKind));
   ID.AddPointer(PT);
   ID.AddPointer(sRegion);
 }
 
 void CXXThisRegion::Profile(llvm::FoldingSetNodeID &ID) const {
   CXXThisRegion::ProfileRegion(ID, ThisPointerTy, superRegion);
 }
 
 void ObjCIvarRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
                                    const ObjCIvarDecl *ivd,
                                    const MemRegion* superRegion) {
   DeclRegion::ProfileRegion(ID, ivd, superRegion, ObjCIvarRegionKind);
 }
 
 void DeclRegion::ProfileRegion(llvm::FoldingSetNodeID& ID, const Decl *D,
                                const MemRegion* superRegion, Kind k) {
-  ID.AddInteger((unsigned) k);
+  ID.AddInteger(static_cast<unsigned>( k));
   ID.AddPointer(D);
   ID.AddPointer(superRegion);
 }
 
 void DeclRegion::Profile(llvm::FoldingSetNodeID& ID) const {
   DeclRegion::ProfileRegion(ID, D, superRegion, getKind());
 }
 
 void VarRegion::Profile(llvm::FoldingSetNodeID &ID) const {
   VarRegion::ProfileRegion(ID, getDecl(), superRegion);
 }
 
 void SymbolicRegion::ProfileRegion(llvm::FoldingSetNodeID& ID, SymbolRef sym,
                                    const MemRegion *sreg) {
-  ID.AddInteger((unsigned) MemRegion::SymbolicRegionKind);
+  ID.AddInteger(static_cast<unsigned>( MemRegion::SymbolicRegionKind));
   ID.Add(sym);
   ID.AddPointer(sreg);
 }
 
 void SymbolicRegion::Profile(llvm::FoldingSetNodeID& ID) const {
   SymbolicRegion::ProfileRegion(ID, sym, getSuperRegion());
 }
 
std::string MemRegion::getString() const {
   return os.str();
 }
 
 void MemRegion::dumpToStream(raw_ostream &os) const {
   os << "<Unknown Region>";
 }
 
 void AllocaRegion::dumpToStream(raw_ostream &os) const {
-  os << "alloca{" << (const void*) Ex << ',' << Cnt << '}';
+  os << "alloca{" << static_cast<const void*>( Ex ) << ',' << Cnt << '}';
 }
 
 void FunctionCodeRegion::dumpToStream(raw_ostream &os) const {
   os << "code{" << getDecl()->getDeclName().getAsString() << '}';
 }
 
 void BlockCodeRegion::dumpToStream(raw_ostream &os) const {
-  os << "block_code{" << (const void*) this << '}';
+  os << "block_code{" << static_cast<const void*>( this ) << '}';
 }
 
 void BlockDataRegion::dumpToStream(raw_ostream &os) const {
   os << "block_data{" << BC;
   os << "; ";
   for (BlockDataRegion::referenced_vars_iterator
          I = referenced_vars_begin(),
          E = referenced_vars_end(); I != E; ++I)
     os << "(" << I.getCapturedRegion() << "," <<
                  I.getOriginalRegion() << ") ";
   os << '}';
 }
 
 void CompoundLiteralRegion::dumpToStream(raw_ostream &os) const {
   // FIXME: More elaborate pretty-printing.
-  os << "{ " << (const void*) CL <<  " }";
+  os << "{ " << static_cast<const void*>( CL ) <<  " }";
 }
 
 void CXXTempObjectRegion::dumpToStream(raw_ostream &os) const {
   os << "temp_object{" << getValueType().getAsString() << ','
-     << (const void*) Ex << '}';
+     << static_cast<const void*>( Ex ) << '}';
 }
 
 void CXXBaseObjectRegion::dumpToStream(raw_ostream &os) const {
   os << "base{" << superRegion << ',' << getDecl()->getName() << '}';
 }
 
 void CXXThisRegion::dumpToStream(raw_ostream &os) const {
   os << "this";

 
 //===----------------------------------------------------------------------===//
 // MemRegionManager methods.
 //===----------------------------------------------------------------------===//
 
 template <typename REG>
 const REG *MemRegionManager::LazyAllocate(REG*& region) {
   if (!region) {
-    region = (REG*) A.Allocate<REG>();
+    region = A.Allocate<REG>();
     new (region) REG(this);
   }
 
   return region;
 }
 
 template <typename REG, typename ARG>
 const REG *MemRegionManager::LazyAllocate(REG*& region, ARG a) {
   if (!region) {
-    region = (REG*) A.Allocate<REG>();
+    region = A.Allocate<REG>();
     new (region) REG(this, a);
   }
 
   return region;
 }
 
 const StackLocalsSpaceRegion*
 MemRegionManager::getStackLocalsRegion(const StackFrameContext *STC) {
MemRegionManager::getElementRegion(QualType elementType, NonLoc Idx,
   llvm::FoldingSetNodeID ID;
   ElementRegion::ProfileRegion(ID, T, Idx, superRegion);
 
   void *InsertPos;
   MemRegion* data = Regions.FindNodeOrInsertPos(ID, InsertPos);
   ElementRegion* R = cast_or_null<ElementRegion>(data);
 
   if (!R) {
-    R = (ElementRegion*) A.Allocate<ElementRegion>();
+    R = A.Allocate<ElementRegion>();
     new (R) ElementRegion(T, Idx, superRegion);
     Regions.InsertNode(R, InsertPos);
   }
 
   return R;
 }
 
 const FunctionCodeRegion *
case FieldRegionKind: {
       // Don't bother calculating precise offsets if we already have a
       // symbolic offset somewhere in the chain.
       if (SymbolicOffsetBase)
         continue;
 
       // Get the field number.
       unsigned idx = 0;
       for (RecordDecl::field_iterator FI = RD->field_begin(),
-             FE = RD->field_end(); FI != FE; ++FI, ++idx)
+             FE = RD->field_end(); FI != FE; ++FI, ++idx) {
         if (FR->getDecl() == *FI)
           break;
-
+      }
       const ASTRecordLayout &Layout = getContext().getASTRecordLayout(RD);
       // This is offset in bits.
       Offset += Layout.getFieldOffset(idx);
       break;
     }
     }
   }
 

 }
 
 void BlockDataRegion::LazyInitializeReferencedVars() {
   if (ReferencedVars)
     return;
 
   AnalysisDeclContext *AC = getCodeRegion()->getAnalysisDeclContext();
   const auto &ReferencedBlockVars = AC->getReferencedBlockVars(BC->getDecl());
-  auto NumBlockVars =
+  const auto NumBlockVars =
       std::distance(ReferencedBlockVars.begin(), ReferencedBlockVars.end());
 
   if (NumBlockVars == 0) {
     ReferencedVars = (void*) 0x1;
     return;
   }
 
   MemRegionManager &MemMgr = *getMemRegionManager();
void RegionAndSymbolInvalidationTraits::setTrait(const MemRegion *MR,
   assert(MR);
   if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(MR))
     setTrait(SR->getSymbol(), IK);
   else
     MRTraitsMap[MR] |= IK;
 }
 
 bool RegionAndSymbolInvalidationTraits::hasTrait(SymbolRef Sym,
-                                                 InvalidationKinds IK) {
+                                                 InvalidationKinds IK) const {
   const_symbol_iterator I = SymTraitsMap.find(Sym);
   if (I != SymTraitsMap.end())
     return I->second & IK;
 
   return false;
 }
 
 bool RegionAndSymbolInvalidationTraits::hasTrait(const MemRegion *MR,
-                                                 InvalidationKinds IK) {
+                                                 InvalidationKinds IK) const {
   if (!MR)
     return false;
 
   if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(MR))
     return hasTrait(SR->getSymbol(), IK);
 
   const_region_iterator I = MRTraitsMap.find(MR);
   if (I != MRTraitsMap.end())
     return I->second & IK;
 
   return false;
 }