[PATCH] D131464: [test] Make tests pass regardless of gnu++14/gnu++17 default

Wed Aug 10 18:44:58 PDT 2022

dblaikie added a comment.

> I use something like
>
>   // RUN: %clang_cc1 %s -fsyntax-only -verify=expected,precxx17 -std=c++14 -fdata-sections -fcolor-diagnostics
>   // RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++17 -fdata-sections -fcolor-diagnostics
>   
>   ...
>     TypedefAligned4 TA8c = TA8a + TA8b;  // expected-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'operator+' may result in an unaligned pointer access}}
>                                          // expected-warning at -1 {{passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'operator+' may result in an unaligned pointer access}}
>                                          // precxx17-warning at -2 {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'StructAligned8' may result in an unaligned pointer access}}
>
> Since every RUN line uses -std=.
> Pros: using `-verify` avoids `#if` directives which make the result look better.
> Cons: when we upgrade to C++20, 23, ..., the tests will become less relevant.

Yeah, that's the main/a major concern as @aaron.ballman  has mentioned.

I think in this case a regex over either would be acceptable, since the test probably wasn't intending to test the particular wording for a particular version (presumably if this is the place where this separate wording is uniquely used (rather than some generic diagnostic infrastructure change) then it's got a C++17 test already)

> Discussion
> ----------
>
> Do we want to prefer `#if` in all cases? They work well with `-verify` tests but not so well with `FileCheck` since `FileCheck` does not ignore preprocessor skipped ranges.

I think where `#if` works well, it seems good to prefer it, yeah.

> Do all `CodeGenCXX` tests have to be fixed?

I think so.

================
Comment at: clang/test/AST/sourceranges.cpp:1-2
-// RUN: %clang_cc1 -triple i686-mingw32 -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++14 -triple i686-mingw32 -ast-dump %s | FileCheck %s
 // RUN: %clang_cc1 -triple i686-mingw32 -std=c++1z -ast-dump %s | FileCheck %s -check-prefix=CHECK-1Z

----------------
MaskRay wrote:
> aaron.ballman wrote:
> > I assume this change is because we're testing c++17 on the next RUN line and you want to be sure we still get the old test coverage?
> > 
> > The trouble is, when we bump from C++17 to C++20, we'll lose the test coverage for the latest standard.
> > 
> > (Not your problem per se, but I continue to think we have a lit deficiency where we need some facilities to say "run this test in C++N and later", "run this test in C89 through CN", or "run this test in all C++ language modes", etc.)
> `FileCheck` does not ignore preprocessor skipped ranges, so it is very difficult to work with both C++14/C++17 defaults, if our intention is to not touch such tests in the default changing patch.
> 
> I think the best strategy is to do another pass to clean up the tests after the transition, not trying to address everything in this patch.
> I think the best strategy is to do another pass to clean up the tests after the transition, not trying to address everything in this patch.

Not everything needs to be addressed in this patch - these cleanups can be in several preliminary patches that are isolated and not dependent on the switch to C++17.

It looks like in this case there's one `CHECK-1Z` that's out of place (line 111, test seems to pass OK even if that's a plain `CHECK` so I guess it probably should be) - and the rest are all at the end, which could be moved to a separate file and done with a hardcoded C++17 target. (but yeah, after the default switch we might end up wanting to remove the hardcoded 17 (allowing the test to become "17 and above"), move more test coverage over to here and leave whatever's 14 specific in the old test)

(maybe as a separate pass, though, to your point - we might be able to go through and remove `-std=c++17` from tests once that's the default, so the tests are forwards compatible to the next language version, most likely - but, again, a broader LIT feature that @aaron.ballman is referring to, where tests could specify which configurations (beyond language versions, this could also help cover things like... oh, right here: https://reviews.llvm.org/D125946, tests could be assumed to be correct for clang-repl too, and some mode where clang-repl's used instead of clang for broad-scale testing of clang-repl)

================
Comment at: clang/test/CXX/except/except.spec/p9-dynamic.cpp:1
-// RUN: %clang_cc1 -no-opaque-pointers %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -fcxx-exceptions -fexceptions | FileCheck %s --check-prefixes=CHECK,CHECK-PRE17
+// RUN: %clang_cc1 -std=c++14 -no-opaque-pointers %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -fcxx-exceptions -fexceptions | FileCheck %s --check-prefixes=CHECK,CHECK-PRE17
 // RUN: %clang_cc1 -no-opaque-pointers %s -triple=x86_64-apple-darwin10 -std=c++17 -Wno-dynamic-exception-spec -emit-llvm -o - -fcxx-exceptions -fexceptions | FileCheck %s --check-prefixes=CHECK,CHECK-17
----------------
MaskRay wrote:
> aaron.ballman wrote:
> > Same question here as above
> The check prefixes assume that this is for C++14.
so in this case the 17 test could become language-agnostic after the version default change?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131464