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