-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Update test added in #154418 to work when the default is C++20. #154463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-clang Author: None (dyung) ChangesThe test added for #154418 specifically tests for a warning that the compiler should emit informing the user that a feature is a C++20 extension, but the compiler does not emit that when the default is C++20, so we should skip that test. This change checks whether the C++ default standard is less than C++20, and if so, skips the test. Full diff: https://github.com/llvm/llvm-project/pull/154463.diff 1 Files Affected:
diff --git a/clang/test/SemaTemplate/nested-name-spec-template.cpp b/clang/test/SemaTemplate/nested-name-spec-template.cpp
index c99e1eed456cc..833f76218212a 100644
--- a/clang/test/SemaTemplate/nested-name-spec-template.cpp
+++ b/clang/test/SemaTemplate/nested-name-spec-template.cpp
@@ -168,7 +168,7 @@ namespace unresolved_using {
template struct C<int>;
} // namespace unresolved_using
-#if __cplusplus >= 201703L
+#if (__cplusplus >= 201703L && __cplusplus < 202002L)
namespace SubstTemplateTypeParmPackType {
template <int...> struct A {};
|
Any other alternative would be preferable over skipping the test altogether. My suggestion is to disable that warning on the command line and then simply delete that expected-warning line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add -std=c++17
and change -verify
to -verify=expected,cxx17
so that we can use // cxx17-warning {{...}}
instead of adding #if
block for some test cases. See https://clang.llvm.org/docs/InternalsManual.html#testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also better than disabling the test, but we don't need to test that we produce this warning, this test is not about that. It's a regression test for a crash and the warning is irrelevant.
Oh thanks, I didn't realize this was just testing for a crash rather than conformance type warning. I'll update the test to do as you suggest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks!
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/20819 Here is the relevant piece of the build log for the reference
|
The test added for #154418 specifically tests for a warning that the compiler should emit informing the user that a feature is a C++20 extension, but the compiler does not emit that when the default is C++20, so we should skip that test. This change checks whether the C++ default standard is less than C++20, and if so, skips the test.