[PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

I guess this is a reasonable refactoring. Although someone with different tastes might come along and inline it back in since the two extracted functions each only have single callers.


================
Comment at: llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1185
@@ +1184,3 @@
+  /// getMemRegionGloballyStored - Retrieve or create the memory region
+  ///  associated with a specified globally stored, non-static local, VarDecl.
+  const MemRegion *getMemRegionGloballyStored(const VarDecl *D);
----------------
I think describing this as taking a "globally stored, non-static local, VarDecl" is ambiguous. This method operates on global variables. It does not act on local variables (static or otherwise). How about "Retrieve or create the memory region associated with a VarDecl for a global variable."

Also, the recommended style these days is to not prefix the doc comment with the name of the member, so I would remove "getMemRegionGloballyStored - " even though getVarRegion() has the same thing. The doc comment style in this file is sufficiently mismatched that I think it is better to do the now-recommend thing rather than try to match its surrounding context.

================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:769
@@ -768,4 +768,3 @@
 
-const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
-                                                const LocationContext *LC) {
-  const MemRegion *sReg = nullptr;
+const MemRegion* MemRegionManager::getMemRegionGloballyStored(const VarDecl *D) {
+  assert(D->hasGlobalStorage());
----------------
a.sidorin wrote:
> `get[Global/StaticLocal]MemSpaceForVariable`?
The rest of the method in class follow a pattern of getAdjectiveNoun, so I would suggest something like getGlobalVarRegion() and getLocalVarRegion()

================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:792
@@ -783,12 +791,3 @@
 
-    // Treat other globals as GlobalInternal unless they are constants.
-    } else {
-      QualType GQT = D->getType();
-      const Type *GT = GQT.getTypePtrOrNull();
-      // TODO: We could walk the complex types here and see if everything is
-      // constified.
-      if (GT && GQT.isConstQualified() && GT->isArithmeticType())
-        sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
-      else
-        sReg = getGlobalsRegion();
-    }
+const MemRegion* MemRegionManager::getMemRegionStaticLocal(const VarDecl *D, const LocationContext *LC) {
+  // FIXME: Once we implement scope handling, we will need to properly lookup
----------------
It looks to me like this function operates on both locals *and* static locals. I would change the name to reflect that.

================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:848
@@ +847,3 @@
+
+  // Finally handle static locals.
+  } else {
----------------
This comment was wrong before and very misleading. I would just remove it so no one else trips on it!


http://reviews.llvm.org/D16873