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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

It will take some time to fix all tests properly. Let's have a guideline how to fix them properly. I tried fixing some using several patterns in the last revision. I didn't fix all, because it would likely lead to an unsatisfactory result and I would spend more time :)

A construct which causes an error with C++17 onwards
----------------------------------------------------

I keep the existing `RUN:` lines and use something like

  #if __cplusplus < 201703L
  void* operator new[](std::size_t) throw(std::bad_alloc); 
  #endif

it the construct appears tested elsewhere or

  register int ro; // expected-error {{illegal storage class on file-scoped variable}}
  #if __cplusplus >= 201703L
  // expected-error at -2 {{ISO C++17 does not allow 'register' storage class specifier}}
  #elif __cplusplus >= 201103L
  // expected-warning at -4 {{'register' storage class specifier is deprecated}}
  #endif

if the test file appears the proper place to test the construct.

Pre-C++17 and C++17 have different results
------------------------------------------

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.

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.
Do all `CodeGenCXX` tests have to be fixed?



================
Comment at: clang/test/AST/ast-dump-undeduced-expr.cpp:1
-// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck %s
+// RUN: not %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck %s
 
----------------
aaron.ballman wrote:
> What from this test requires C++14?
The C++17 diagnostics uses `static inline constexpr` while pre-C++17 uses `static constexpr`. Used a regex instead.


================
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
 
----------------
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.


================
Comment at: clang/test/Analysis/blocks.m:2
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -fblocks -verify -Wno-strict-prototypes %s
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -fblocks -verify -x objective-c++ %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -fblocks -verify -x objective-c++ -std=c++14 %s
 
----------------
aaron.ballman wrote:
> What from this test is invalid in C++17?
Added `__attribute__((__nothrow__))` in C++17 mode.


================
Comment at: clang/test/CXX/except/except.spec/p2-dynamic-types.cpp:1
-// RUN: %clang_cc1 -fexceptions -fcxx-exceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fexceptions -fcxx-exceptions -fsyntax-only -verify -std=c++14 %s
 
----------------
aaron.ballman wrote:
> Why not pass `-Wno-dynamic-exception-spec` instead of limiting the test (we support dynamic exception specifications in newer language modes and it would be good to not lose that test coverage there).
TIL `-Wno-dynamic-exception-spec`. Adopted.


================
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
----------------
aaron.ballman wrote:
> Same question here as above
The check prefixes assume that this is for C++14.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131464