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

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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;
----------------
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)


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

https://reviews.llvm.org/D120397