[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:1470
+    /// diagnostics should be emitted.
+    SmallVector<Decl *, 4> DeclsToCheckForDeferredDiags;
+
----------------
This needs to be saved and restored in modules / PCH.


================
Comment at: clang/lib/Sema/Sema.cpp:1472
   if (HasWarningOrError && ShowCallStack)
-    emitCallStackNotes(S, FD);
+    emitCallStackNotes(*this, FD);
+}
----------------
Hmm.  I know this is existing code, but I just realized something.  I think it's okay to not emit the notes on every diagnostic, but you might want to emit them on the first diagnostic from a function instead of after the last.  If the real bug is that the program is using something it's not supposed to use, and there are enough errors in that function to reach the error limit, then the diagnostics emitter will associate these notes with a diagnostic it's suppressing and so presumably suppress them as well, leaving the user with no way to find this information.


================
Comment at: clang/lib/Sema/Sema.cpp:1494
+      visitUsedDecl(E->getLocation(), FD);
+    }
+  }
----------------
This needs to trigger if you use a variable with delayed diagnostics, too, right?

When you add these methods to `UsedDeclVisitor`, you'll be able to remove them here.


================
Comment at: clang/lib/Sema/Sema.cpp:1512
+    Inherited::VisitCapturedStmt(Node);
+  }
+
----------------
Should this also go in the base `UsedDeclVisitor`?  I'm less sure about that because the captured statement is really always a part of the enclosing function, right?  Should the delay mechanism just be looking through local sub-contexts instead?


================
Comment at: clang/lib/Sema/UsedDeclVisitor.h:21
+template <class Derived>
+class UsedDeclVisitor : public EvaluatedExprVisitor<Derived> {
+protected:
----------------
Could you add this in a separate patch?


================
Comment at: clang/lib/Sema/UsedDeclVisitor.h:30
+
+  Derived &asImpl() { return *static_cast<Derived *>(this); }
+
----------------
There should definitely be cases in here for every expression that uses a declaration, including both `DeclRefExpr` and `MemberExpr`.  Those might be overridden in subclasses, but that's their business; the default behavior should be to visit every used decl.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70172/new/

https://reviews.llvm.org/D70172