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

[Alexander Riccio via cfe-commits]

ariccio marked 9 inline comments as done.
ariccio added a comment.

Whoops, I didn't //submit// my comments.


================
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);
----------------
dcoughlin wrote:
> 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.
I like that. Done.

================
Comment at: llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1186
@@ +1185,3 @@
+  ///  associated with a specified globally stored, non-static local, VarDecl.
+  const MemRegion *getMemRegionGloballyStored(const VarDecl *D);
+
----------------
a.sidorin wrote:
> ariccio wrote:
> > a.sidorin wrote:
> > > How about make these helper functions return `const MemSpaceRegion *` to make their signatures more meaningful?
> > Would that change their behavior functionally?
> No, it will not change it. You will need to change `static_cast` type to `const MemSpaceRegion *`, however (lines 809-810). There is `const MemSpaceRegion *` and its children classes everywhere in return statements already.
What're the `static_cast<const MemRegion*>`s for anyways? Shouldn't the `const StackArgumentsSpaceRegion*`s convert to `const MemRegion*` automatically?

`StackArgumentsSpaceRegion` derives from `StackSpaceRegion`:

`class StackArgumentsSpaceRegion : public StackSpaceRegion`

...`StackSpaceRegion` derives from `MemSpaceRegion`:

`class StackSpaceRegion : public MemSpaceRegion`

...and `MemSpaceRegion` derives from `MemRegion`:

`class MemSpaceRegion : public MemRegion`

...which I think should work, because of the kooky rules of covariant return types (or something)?

================
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());
----------------
ariccio wrote:
> ariccio wrote:
> > dcoughlin wrote:
> > > 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()
> > Ahh, that might make more sense. I did this refactoring without any sense of context, as the unnecessary complexity was a hindrance thereto.
> > 
> How about `getGlobalMemSpaceForGlobalVariable`? I'm hesitant to drop the second global, to be clear about the scope of the variable that's stored globally. Is that a reasonable concern, or is that redundant?
Whatever. I've changed the global one to `getGlobalVarRegion`.

================
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
----------------
dcoughlin wrote:
> It looks to me like this function operates on both locals *and* static locals. I would change the name to reflect that.
So, `getMemRegionStaticLocalOrLocal`?

================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:800
@@ -803,1 +799,3 @@
+  if (V.is<const VarRegion*>())
+    return V.get<const VarRegion*>();
 
----------------
a.sidorin wrote:
> Oops.
> Lines above should stay in the caller function or be refactored. Otherwise, we'll get the subregion of `V.get<const VarRegion *` instead of returning it directly. This looks like a behaviour change. (And this is the only place where return type is not a `const MemSpaceRegion *`, btw).
> This issue may also obsolete my comments about return type.
Nice catch! That's exactly why we do code review :)

>This issue may also obsolete my comments about return type.

We might not want to change the return type, I see.


http://reviews.llvm.org/D16873