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

[Leonard Chan via Phabricator via cfe-commits]

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