[PATCH] D85933: Don't track consteval references for dependent members

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1761
+      llvm::SaveAndRestore<bool> DisableIITracking(
+          S.RebuildingImmediateInvocation, true);
+
----------------
Tyker wrote:
> I don't think RebuildingImmediateInvocation should be used here since we are not rebuilding immediate invocations
> 
> setSuppressAllDiagnostics or TentativeAnalysisScope seems more adapted.
These don't actually work for this use case due to the queuing nature of the diagnostic. Given the hacky nature of this assertion as it stands, I didn't want to further pollute the code base.

I'm open to suggestions. If you're proposing making `MarkDeclRefReferenced` not queue in response to diagnostic suppression, that doesn't seem totally unreasonable; we'll just have to add the additional condition to check the diagnostic state in `MarkDeclRefReferenced`.


================
Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:131
+  consteval A() = default;
+  consteval ~A() = default; // expected-error {{destructor cannot be declared consteval}}
+};
----------------
Tyker wrote:
> could you check that this diagnostic and those like it doesn't appear when the struct A isn't instantiated.
I duplicated this namespace without any of the instantiation, and it does seem to trigger the diagnostic "destructor cannot be declared consteval" -- all other diagnostics are silent. Is that particularly undesirable? 

Also, WRT testing. would that the best option here (having a duplicated namespace, "dependent_addressing_uninstantiated" -- without the line triggering instantiation)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85933