Paths
| Differential D124434
[Clang][Test] Run tests in C++14 mode explicitly. AbandonedPublic Authored by junaire on Apr 25 2022, 11:02 PM.
Details Summary Some tests are not specify their langauge standard but using the default Related Issue: https://github.com/llvm/llvm-project/issues/55077
Diff Detail
Event TimelineComment Actions @rsmith @aaron.ballman - might be especially interesting to know your thoughts on the C++ chapter-based testing and what the intent there is as clang changes default versions/new versions are added. (& also whether there's an approach (like moving the default language into the driver) that'd be worthwhile to lower the maintenance cost of these sort of migrations without compromising on test coverage/ending up with all the test coverage testing previous versions of the language and not what everyone's using in the wild)
Comment Actions
In general, my concern with the this patch is that it loses test coverage by specifying an explicit language mode. We typically prefer to fix the tests so that they can work in any language mode (and perhaps add additional RUN lines in the process to do so). However, one thought I've had a few times in recent history is that I'd like to see an adjustment to the way lit tests are run. Ultimately, what I'd like to see is that lit (perhaps as an opt-in option) have the ability to test a single test file under every language mode the test file is valid for. Then we can modify the RUN lines of tests with a lit %magic_variable to specify what those language modes are. My thinking is that lit would take note of the use of the magic variable (or absence of any -std mode) and automatically generate a RUN line for each standard mode based on the template using the magic variable. This would give us the flexibility to write tests that work for any given language mode without losing test coverage. e.g., // This gets run under C++14 and up, but not earlier versions // RUN: %clang_cc1 -fsyntax-only -std=%cxx14+ foo.cpp // This gets run under C++14 through C++20, but not earlier or later versions // RUN: %clang_cc1 -fsyntax-only -std=%cxx14-%cxx20 foo.cpp // This gets run under all C++ language modes (based on the file extension) because no -std was specified // RUN: %clang_cc1 -fsyntax-only foo.cpp // This gets run only in C++11 mode, as it is today // RUN: %clang_cc1 -fsyntax-only -std=c++11 foo.cpp If the lit behavior is opt-in and the user doesn't opt into it, then we can replace any lit magic variable with the latest standards mode in the given range or the current default language mode as appropriate. I think this sort of flexibility would be really helpful for many of our tests and I think it would help shake out bugs in Clang that result from specifying an explicit language mode. However, I have no idea if others think this is a good idea, whether it's possible/easy to do in lit, etc. As for the chapter-based testing... I am not convinced that the experiment has panned out well. 1) it only works for C++ where there are stable names, but it's not usable for C where everything is based on clause numbers that change frequently, 2) even in C++ it falls down somewhat regularly because we name the test files after a paragraph number, and p3 in C++N may be p2 or p4 in C++N+1, 3) we don't actively encourage people to add tests to the chapters (maybe that's my fault as a reviewer and I should encourage that more strongly?). So I'm not certain I'd be sad if we distributed the chapter tests out amongst the other test directories or froze the chapter tests in amber for a specific language version. Comment Actions
OK, I'll do this. But I guess it is not sort of trivial work and will take plenty of time. Comment Actions
Oh, please do not feel obligated to do that work yourself! Not only is it nontrivial and likely to take a fair amount of effort, I'm not even certain if other people think it's a good idea or not. I see this more as a start of a discussion as to how we want to handle this. I think when C and C++ were on 10+ year release cycles, this model was a bit more palatable, but now that the releases are shorter (every 3 yrs for C++ at the least) and there's more interplay between them (extensions ported to older language modes, etc) we might want something different. Comment Actions
Yeah, I have mixed feelings - I think at least in theory, C++ tries to be mostly backwards compatible and so old tests should run successfully in new language modes - and I guess for the most part they do (even though there's a lot of failures, they're few relative to the total number of language-version-unspecified tests we have, no doubt). Anyone have a rough sense of what we did the last couple of language default changes? (not necessarily generalizable, as you say, @aaron.ballman - as the release pace has picked up, trying to figure out what's a suitable/sustainable approach now) Comment Actions
The trouble is, C++ assumes its changes will be backwards compatible, but many proposals are adopted with zero implementation experience, so the difference between theory and practice is getting wider rather than narrower over time. But even setting that aside, Clang adds quite a few things as extensions in older language modes as well, which can sometimes have surprising impacts too when you peg a test to only one standards mode.
The last time we changed was Clang 6.0 and it looks like we did not change tests to add an explicit -std very often (we didn't have to touch many files at all): https://github.com/llvm/llvm-project/commit/36bb6d5d467350fa8056237cde4c5147a82f5938 I think that was the first time we changed the C++ language default. Comment Actions
Sure enough, yeah. I guess coming back to your other point about restructing the way all this testing works (be a pity to gate the default change on this work as it sounds like a big project) - yeah, it'd be nice to be able to declare a test as "language version neutral" or "in this version and above" (or, I guess "only in these versions and no others") - and have some mode in the lit build that can then handle running it in all known versions - and maybe the developer default could be "run in the most recent shipped version only" for speed (if the speed is noticable) and leave it to buildbots to run in all the language versions.
Hmm, fair enough - not a lot to generalize from, then. (gees, I'm surprised that was only 5 years ago that we switched from the `98 default... ) Comment Actions
Yeah, that's basically what I was envisioning; it may make build bots slower, but any expansion of test coverage leads to that outcome, so I don't see it as being a bad thing so long as we keep the developer experience reasonable. We might want to do something on the build bots to help folks know "to reproduce, you may need to run lit with this extra parameter so all tests are run" or some other marker to help folks who break a buildbot know what's going on.
Revision Contents
Diff 425123 clang/test/AST/ast-dump-openmp-begin-declare-variant_11.c
clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
clang/test/AST/ast-dump-undeduced-expr.cpp
clang/test/AST/sourceranges.cpp
clang/test/Analysis/blocks.m
clang/test/CXX/basic/basic.stc/basic.stc.dynamic/p2.cpp
clang/test/CXX/class.access/class.friend/p1.cpp
clang/test/CXX/class/class.friend/p2.cpp
clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p2.cpp
clang/test/CXX/except/except.spec/p2-dynamic-types.cpp
clang/test/CXX/except/except.spec/p9-dynamic.cpp
clang/test/CXX/stmt.stmt/stmt.select/p3.cpp
clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp
clang/test/CXX/temp/temp.res/temp.local/p3.cpp
clang/test/CodeGen/typedef_alignment_mismatch_warning.cpp
clang/test/CodeGenCXX/align-avx-complete-objects.cpp
clang/test/CodeGenCXX/copy-constructor-elim-2.cpp
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
clang/test/CodeGenCXX/debug-info-template-partial-specialization.cpp
clang/test/CodeGenCXX/exception-spec-decay.cpp
clang/test/CodeGenCXX/exceptions-cxx-ehsc.cpp
clang/test/CodeGenCXX/exceptions-no-rtti.cpp
clang/test/CodeGenCXX/global-init.cpp
clang/test/CodeGenCXX/no-exceptions.cpp
clang/test/CodeGenCXX/override-bit-field-layout.cpp
clang/test/CodeGenCXX/override-layout.cpp
clang/test/CodeGenCXX/reference-temporary-ms.cpp
clang/test/CodeGenCXX/rtti-linkage.cpp
clang/test/Layout/ms-x86-vtordisp.cpp
clang/test/Modules/update-exception-spec.cpp
clang/test/OpenMP/declare_mapper_messages.cpp
clang/test/PCH/cxx-functions.cpp
clang/test/Parser/cxx-casting.cpp
clang/test/Parser/cxx-class.cpp
clang/test/Parser/cxx-template-argument.cpp
clang/test/Parser/cxx-template-decl.cpp
clang/test/Parser/cxx1z-nested-namespace-definition.cpp
clang/test/Sema/ms_class_layout.cpp
clang/test/SemaCXX/MicrosoftExtensions.cpp
clang/test/SemaCXX/PR12778.cpp
clang/test/SemaCXX/altivec.cpp
clang/test/SemaCXX/bool.cpp
clang/test/SemaCXX/default2.cpp
clang/test/SemaCXX/exception-spec-no-exceptions.cpp
clang/test/SemaCXX/exceptions.cpp
clang/test/SemaCXX/expressions.cpp
clang/test/SemaCXX/inline.cpp
clang/test/SemaCXX/libstdcxx_is_pod_hack.cpp
clang/test/SemaCXX/linkage2.cpp
clang/test/SemaCXX/member-pointer.cpp
clang/test/SemaCXX/missing-namespace-qualifier-typo-corrections.cpp
clang/test/SemaCXX/new-delete.cpp
clang/test/SemaCXX/static-data-member.cpp
clang/test/SemaCXX/type-definition-in-specifier.cpp
clang/test/SemaCXX/user-defined-conversions.cpp
clang/test/SemaCXX/warn-new-overaligned-3.cpp
clang/test/SemaCXX/warn-new-overaligned.cpp
clang/test/SemaCXX/writable-strings-deprecated.cpp
clang/test/SemaSYCL/zero-length-arrays.cpp
clang/test/SemaTemplate/class-template-id.cpp
clang/test/SemaTemplate/constructor-template.cpp
clang/test/SemaTemplate/explicit-instantiation.cpp
clang/test/SemaTemplate/instantiate-exception-spec.cpp
clang/test/SemaTemplate/instantiation-default-2.cpp
clang/test/SemaTemplate/temp_arg.cpp
clang/test/SemaTemplate/temp_arg_template.cpp
clang/test/SemaTemplate/typename-specifier-3.cpp
clang/unittests/AST/ASTTraverserTest.cpp
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Might be worth knowing the version history of tests like this? (in here and other test files) - perhaps the intent is for this test to check the code is valid in all language versions & so should grow more explicit version lines as the language evolves?