leonardchan added inline comments.
================
Comment at: lib/Parse/ParseExpr.cpp:1126
+
+ Actions.StartCheckingNoDeref();
+
----------------
rsmith wrote:
> This parser-driven start/stop mechanism will not work in C++ templates.
> Instead, can you remove the "start" part and check the noderef exprs as part
> of popping the ExprEvaluationContextRecord?
I'm not sure if I should remove the the start and stop methods because for a
regular C program, the Push/PopExprEvaluationContextRecord functions don't seem
to be called, and even when they are called in C++, the initial record that
exists on the stack isn't popped at all.
Since pending noderef expressions are still parsed and added to the last record
during template instantiation, doing another check when popping covers all
noderef exprs added during instantiation.
================
Comment at: lib/Sema/SemaExpr.cpp:14230-14242
+class DeclRefFinder : public ConstEvaluatedExprVisitor<DeclRefFinder> {
+public:
+ typedef ConstEvaluatedExprVisitor<DeclRefFinder> Inherited;
+
+ DeclRefFinder(ASTContext &Ctx) : Inherited(Ctx) {}
+
+ void VisitDeclRefExpr(const DeclRefExpr *E) { DeclRef = E; }
----------------
rsmith wrote:
> rsmith wrote:
> > I don't see any reason to assume that the `DeclRefExpr` found by this
> > visitor will be the one you're interested in (the one the `noderef`
> > attribute came from).
> >
> > How about instead you walk over only expressions that you *know* preserve
> > `noderef`, and if you can't find a declaration as the source of the
> > `noderef` attribute, produce a differently-worded diagnostic that doesn't
> > give a variable name?
> This comment is marked done but has not been addressed.
My bad. My response to this was adding a check in `StopCheckingNoDerefAndWarn`
where we check if the `DeclRefExpr` is `nullptr` and throw the appropriate
error if it is found or not.
Although I can't come up with an example where this doesn't get the declref we
want, since the expressions we pass to this visitor are all our marked noderef
expressions (which we check via the attribute on the type). So even if we have
a complex expression that has multiple declrefs, all noderef declrefs should be
checked by this.
================
Comment at: lib/Sema/SemaExpr.cpp:4291
+ const QualType &BaseTy = Base->getType();
+ QualType ElemTy;
+ if (const auto *ArrayTy = dyn_cast<ArrayType>(BaseTy))
----------------
rsmith wrote:
> Did you mean to use ElemTy somewhere below? (I'd guess you intended to check
> whether it's marked noderef?)
Forgot to remove this.
================
Comment at: lib/Sema/SemaType.cpp:4816
+
+ if (const auto *AT = dyn_cast<AttributedType>(T))
+ IsNoDeref = AT->getAttrKind() == AttributedType::attr_noderef;
----------------
rsmith wrote:
> rsmith wrote:
> > Move this after the check below (directly setting ExpectNoDerefChunk
> > instead), and remove the unnecessary IsNoDeref variable.
> This is not correct: there could be multiple attributes at this level. You
> could fix this locally either by looping through the attributes on the type
> or iterating through the ParsedAttrs on the chunk. But I think my preference
> would be to store state indicating that we have an unvalidated noderef
> attribute on the TypeProcessingState at the point where you create the
> AttributedType, and check and clear that flag here when creating a type chunk.
After https://reviews.llvm.org/D50526, it seems that the attribute can also be
applied in `ConvertDeclSpecToType`, so I added another check for the attributes
there.
Repository:
rC Clang
https://reviews.llvm.org/D49511
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
