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

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Responded to comments.

Will happily make changes when questions are answered.


================
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:
> How about make these helper functions return `const MemSpaceRegion *` to make their signatures more meaningful?
Would that change their behavior functionally?

================
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`?
Ahh, that might make more sense. I did this refactoring without any sense of context, as the unnecessary complexity was a hindrance thereto.


================
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:
> a.sidorin wrote:
> > `get[Global/StaticLocal]MemSpaceForVariable`?
> 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?

================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:843
@@ +842,3 @@
+                                                const LocationContext *LC) {
+  const MemRegion *sReg = nullptr;
+
----------------
a.sidorin wrote:
> `const MemSpaceRegion *StorageSpace?`
Same question as above: Would that change their behavior functionally?


(if not, then I'll happily change it)


http://reviews.llvm.org/D16873