[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman removed a reviewer: clang-language-wg.
aaron.ballman added inline comments.


================
Comment at: clang/test/Modules/inconsist-export-template.cpp:1-2
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;
----------------
dblaikie wrote:
> ChuanqiXu wrote:
> > urnathan wrote:
> > > dblaikie wrote:
> > > > This test doesn't appear to test anything - it verifies that this file produces no diagnostics, but not that it has any other/particular behavior.
> > > > 
> > > > Should this be testing codegen to verify that the correct linkage was used on the resulting IR functions?
> > > > 
> > > > Are there other ways of observing the particular language-level linkage of the entities to confirm it's as expected?
> > > yes, this should be a codegen test (clang/test/CodeGenCXX/Modules/cxx20-$something.cpp?
> > The compiler would crash at the test before the patch landed. So I send the patch here to show that the test wouldn't cause the compiler crash.
> My concern is that a "does anything other than crash" is a fairly vague requirement - the existence of a crash points to an unverified codepath, but "doesn't crash" isn't the test that was missing - the test that was missing was for the functionality we'd expect after the crash (that functionality couldn't be/wasn't previously tested, due to the presence of the crash).
> 
> @aaron.ballman perhaps you've got some thoughts/quick opinion here - given the patch, what would you find to be suitable testing (perhaps I'm off-base here and a "no diagnostic" test is suitable, given how various constraints are enforced at AST building time - perhaps the absence of any violation of those constraints adequately tests this patch?) (& if you've got thoughts on the issue of adding test coverage for issues not fixed yet, as discussed below, open to those too)
I think that's a valid concern. We do have more than a handful of tests which are literally "does clang no longer crash" tests, but those tend to be older tests and a bit lower quality over time compared to tests that validate the behavior beyond just crashing. e.g. if the crash is a diagnostic-related crash, we verify the diagnostics are correctly emitted (or not longer incorrectly emitted); if it's related to codegen, we verify that the codegen is valid, etc.

I don't think this should have a CodeGen based on my assumption that this test skips codegen but used to crash. So there was a frontend crash somewhere. Given that the changes relate to setting the linkage for a declaration, I wonder if we can use `static_assert` along with some type traits to determine whether the linkage is set properly purely within the frontend? Alternatively, is the information reflected in an `-ast-dump` (or `-ast-dump=json`) dump so that we could test the linkage that way?


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

https://reviews.llvm.org/D120397