NoQ added inline comments.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051
+
+ const Expr *OriginExpr;
+ unsigned Index;
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > We do not use this at all. However, if I remove it then tests begin to
> > > fail with strange reasons, some of them even crashes at strange points.
> > > It seems that we need this for profiling the subclass.
> > `Profile` is completely essential. Without it different regions will be
> > treated as if it's the same region.
> I know, that was not the question. It is the `OriginExpr` which I tried to
> remove because I do not use it at all, except for profiling. It seems that
> without this field the `Profile` does not work correctly.
Wait, how is it not used? You can't get the region type without `OriginExpr`.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1054-1055
+
+ ParamRegion(const Expr *OE, unsigned Idx, const MemRegion *SReg)
+ : TypedValueRegion(SReg, ParamRegionKind), OriginExpr(OE), Index(Idx) {}
+
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > It looks like you decided to keep `VarRegion` for the top frame.
> >
> > I want that asserted here, in the constructor, so that never to make a
> > mistake on this front.
> Yes, because in the top frame we neither have `CallExpr` nor `Decl`. So we
> should assert here that we are not in the top frame, right? How do we check
> this based on these parameters?
Yes, you did what i meant. The first assertion is redundant because `isa` is
implied by `cast`. Let's also assert that `OE` is non-null.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp:197-198
+ if (const ParamRegion *PR = R->getAs<ParamRegion>())
+ if (const StackArgumentsSpaceRegion *
+ stackReg = dyn_cast<StackArgumentsSpaceRegion>(PR->getMemorySpace()))
+ if (stackReg->getStackFrame() == SFC)
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter.
> So I can remove the inner branch?
In fact you can go even further and change the type of the overridden
`getMemorySpace()` function to return `const StackArgumentsSpaceRegion *`
("covariant return types") so that to drop the `cast` as well.
================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:255
// correspond to the stack frame's function declaration.
assert(VR->getStackFrame() == SFC);
----------------
balazske wrote:
> Is it correct to create `VR` only to have this assert?
It's a valid pattern but it should be followed by `(void)VR;` in order to avoid
unused variable warning in no-assert builds.
================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:187
+QualType ParamRegion::getValueType() const {
+ return getDecl()->getType();
+}
----------------
This doesn't work when the callee is unknown. You need to consult the origin
expr here.
================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+ const Decl *D = getStackFrame()->getDecl();
+
----------------
This doesn't work when the callee is unknown.
================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:579
+ const ParmVarDecl *PVD = getDecl();
+ if (const IdentifierInfo *ID = PVD->getIdentifier()) {
+ os << ID->getName();
----------------
`PVD` may be null.
================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1236
+ const Stmt *CallSite = SFC->getCallSite();
+ if (!CallSite)
+ return getVarRegion(PVD, LC);
----------------
Let's make a stronger assertion:
```lang=c++
if (SFC->inTopFrame())
return getVarRegion(PVD, LC);
assert(CallSite && "Destructors with parameters?");
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79704/new/
https://reviews.llvm.org/D79704
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
