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