[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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