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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
