rsmith added inline comments.
================
Comment at: lib/Parse/ParseExpr.cpp:1126
+
+ Actions.StartCheckingNoDeref();
+
----------------
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?
================
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:
> 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.
================
Comment at: lib/Sema/SemaExpr.cpp:4267
+ } else if (const auto *Member = dyn_cast<MemberExpr>(StrippedExpr)) {
+ // Getting the address of a member expr in the form &(*s).b
+ const Expr *Base = Member->getBase()->IgnoreParenImpCasts();
----------------
You need to loop/recurse here; there could be a sequence of `.` member accesses
to strip.
================
Comment at: lib/Sema/SemaExpr.cpp:4291
+ const QualType &BaseTy = Base->getType();
+ QualType ElemTy;
+ if (const auto *ArrayTy = dyn_cast<ArrayType>(BaseTy))
----------------
Did you mean to use ElemTy somewhere below? (I'd guess you intended to check
whether it's marked noderef?)
================
Comment at: lib/Sema/SemaType.cpp:4209
+ bool IsNoDeref = false;
+ if (const auto *AT = dyn_cast<AttributedType>(T))
+ IsNoDeref = AT->getAttrKind() == AttributedType::attr_noderef;
----------------
This is not correct (there could be multiple attributes on the type). See below.
================
Comment at: lib/Sema/SemaType.cpp:4816
+
+ if (const auto *AT = dyn_cast<AttributedType>(T))
+ IsNoDeref = AT->getAttrKind() == AttributedType::attr_noderef;
----------------
Move this after the check below (directly setting ExpectNoDerefChunk instead),
and remove the unnecessary IsNoDeref variable.
================
Comment at: lib/Sema/SemaType.cpp:4816
+
+ if (const auto *AT = dyn_cast<AttributedType>(T))
+ IsNoDeref = AT->getAttrKind() == AttributedType::attr_noderef;
----------------
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.
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
