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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
