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