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

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

rsmith added a comment.

In https://reviews.llvm.org/D49511#1170693, @leonardchan wrote:

> Done. I opted for not using `ExpressionEvaluationContextRecord` because I don't think I was using it correctly. A few other tests in sema failed when I tried pushing and popping from the stack holding these. I still process expressions as they are parsed though without using AST traversal on every expression.


You shouldn't be adding your own `ExpressionEvaluationContextRecord`s. What I was suggesting is that you store a list of pending `noderef` expressions on the record, and diagnose them when the record is popped (if they've not been removed since). Your current counter-based approach doesn't work very well in the case where we switch to another context while processing an expression (for example, during template instantiation): you'll defer all the diagnostics for the inner construct until the outer construct is complete. Generally global `Sema` state doesn't work very well for this reason.

That said... have you considered changing the specification of your attribute so that you warn on lvalue-to-rvalue conversions instead of warning on dereference-like things with a list of exceptions? That would be both simpler to implement and more precise (and it would naturally extend to C++, where a reference-to-`noderef` would be a reasonable type to support).



================
Comment at: include/clang/Sema/Sema.h:1556
+  unsigned NoDerefCallCount = 0;
+  std::unordered_set<const Expr *> PossibleDerefs;
+
----------------
Do not use `unordered_set` here; see https://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task

`llvm::SmallPtrSet` or `llvm::DenseSet` would be better choices here, but I think just using a `SmallVector` would be fine. 


================
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; }
----------------
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?


================
Comment at: lib/Sema/SemaExpr.cpp:14252-14257
+      const DeclRefExpr *DeclRef = Finder.GetDeclRef();
+      const ValueDecl *Decl = DeclRef->getDecl();
+
+      Diag(E->getExprLoc(), diag::warn_dereference_of_noderef_type)
+          << Decl->getName() << E->getSourceRange();
+      Diag(Decl->getLocation(), diag::note_previous_decl) << Decl->getName();
----------------
This will crash if the `DeclRefFinder` doesn't find a `DeclRefExpr`. What justifies the assumption that it will always succeed?


Repository:
  rC Clang

https://reviews.llvm.org/D49511