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