aaron.ballman added a comment.
In D131464#3713705 <https://reviews.llvm.org/D131464#3713705>, @MaskRay wrote:
> 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@-2 {{ISO C++17 does not allow 'register' storage class
> specifier}}
> #elif __cplusplus >= 201103L
> // expected-warning@-4 {{'register' storage class specifier is deprecated}}
> #endif
>
> if the test file appears the proper place to test the construct.
This makes sense to me for sema tests. For tests which cannot have errors in
them, like codegen tests or analysis tests, I would say this is a case where we
would exhaustively test the old language standards up to C++17 and not beyond
(perhaps with a boilerplate comment about why no newer versions need to be
tested).
> 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@-1 {{passing
> 4-byte aligned argument to 8-byte aligned parameter 1 of 'operator+' may
> result in an unaligned pointer access}}
> // precxx17-warning@-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, we want to avoid making the tests less relevant in the future when
possible. The suggestion from @dblaikie to use a regex in this case makes a lot
of sense to me in the situation where what's being tested is not specifically
the divergence in diagnostic behavior. e.g., if the test is "do we give a
sensible warning here?" then use a regex for this situation, but if the test is
"do we give the correct form of the warning here?" then use `-verify=` and try
to leave a RUN line without a specific language standard on it if possible.
> 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 wouldn't say "in all cases", but certainly in cases where it improves the
readability of the test or ensures we don't lose test coverage. Cases with
`FileCheck` are a problem, but hopefully aren't too prevalent in the Parser and
Sema tests.
> Do all `CodeGenCXX` tests have to be fixed?
Yes, but in that case I expect (at least for errors), it's more a matter of
adding RUN lines for all the relevant modes we need to test (since codegen
tests really should not be caring about warning diagnostic wording, though some
tests still do).
================
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
----------------
dblaikie wrote:
> 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)
> 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.
+1 -- I am imagining the way we want to go forward is to have several patches
handling these cleanups. One can handle cases where we need to add a regex or a
verify prefix, another which splits files into two as needed, another can
introduce new lit features, another can start to use those features, and so on.
This way, the default changing patch can hopefully be relatively small and
narrow in scope. Once it's landed, we probably will want to do even more
cleanup like removing -std=c++17 RUN lines when possible, etc.
Ultimately, I think the lit feature improvements will require us to go through
a lot/most/all-ish of the tests to fix up their RUN lines. When we made the
switch to C++14, I don't think we went through and cleaned up tests that
already specified `-std=c++1y` or `-std=c++14`, so I would not be surprised to
see a bunch of tests that lost future version coverage from that switch as
well. Whether we want to bite the bullet and do that work up front instead of
looking at/touching a lot of tests twice is worth thinking about.
================
Comment at: llvm/utils/lit/lit/llvm/config.py:570
+ clang_std_values = ('98', '11', '14', '17', '20', '2b')
+ def add_stdcxx(s):
+ t = s[8:]
----------------
If we like this approach, we should probably add `add_stdc` as well (not as
part of this patch, we can do all of C++ first, then come back and hit up C
after we've finished).
================
Comment at: llvm/utils/lit/lit/llvm/config.py:573
+ if t.endswith('-'):
+ t += '2b'
+ l = clang_std_values.index(t[0:2])
----------------
Maybe we can use `clang_std_values[-1]` so we don't have to hardcode this?
================
Comment at: llvm/utils/lit/lit/llvm/config.py:579
+ l = h - clang_std_group % (h-l+1)
+ self.config.substitutions.append((s, '-std=c++' +
clang_std_values[l]))
+
----------------
One thing we should consider is whether we want to run in *all* the specified
language modes instead of just the newest mode. This will make running tests
slower because we'll run significantly more of them, and it might get awkward
if a lot of tests change behavior in the different language modes, so I don't
suggest it as part of this patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131464/new/
https://reviews.llvm.org/D131464
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
