-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Clang: Add warning flag for storage class specifiers on explicit specializations #96699
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
Clang: Add warning flag for storage class specifiers on explicit specializations #96699
Conversation
…ializations With the recent fix for this situation in class members (llvm#93873) (for which the fixed code is invalid prior to this patch - making migrating code difficult as it must be in lock-step with the compiler migration, if building with -Werror) it'd be really useful to be able to disable this warning during the compiler migration/decouple the compiler migration from the source fixes. In theory this approach will regress the codebase to the previous non-member cases of this issue that were already being held back by the warning (as opposed to if we carved out the new cases into a separate warning from the existing cases) but I think this'll be so rare and the cleanup so simple, that the extra regressions of disabling the warning broadly won't be too much of a problem. (but if folks disagree, I'm open to making the warning more fine-grained)
@llvm/pr-subscribers-clang Author: David Blaikie (dwblaikie) ChangesWith the recent fix for this situation in class members (#93873) (for In theory this approach will regress the codebase to the previous Full diff: https://github.com/llvm/llvm-project/pull/96699.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 9b37d4bd3205b..8c5cbbc160ba1 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1535,3 +1535,7 @@ def BitIntExtension : DiagGroup<"bit-int-extension">;
// Warnings about misuse of ExtractAPI options.
def ExtractAPIMisuse : DiagGroup<"extractapi-misuse">;
+
+// Warnings about using the non-standard extension having an explicit specialization
+// with a storage class specifier.
+def ExplicitSpecializationStorageClass : DiagGroup<"explicit-specialization-storage-class">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 25a87078a5709..f12121f065783 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5366,7 +5366,7 @@ def err_not_class_template_specialization : Error<
"cannot specialize a %select{dependent template|template template "
"parameter}0">;
def ext_explicit_specialization_storage_class : ExtWarn<
- "explicit specialization cannot have a storage class">;
+ "explicit specialization cannot have a storage class">, InGroup<ExplicitSpecializationStorageClass>;
def err_dependent_function_template_spec_no_match : Error<
"no candidate function template was found for dependent"
" %select{member|friend}0 function template specialization">;
diff --git a/clang/test/Misc/warning-flags.c b/clang/test/Misc/warning-flags.c
index dd73331913c6f..ae14720959a0e 100644
--- a/clang/test/Misc/warning-flags.c
+++ b/clang/test/Misc/warning-flags.c
@@ -18,10 +18,9 @@ This test serves two purposes:
The list of warnings below should NEVER grow. It should gradually shrink to 0.
-CHECK: Warnings without flags (66):
+CHECK: Warnings without flags (65):
CHECK-NEXT: ext_expected_semi_decl_list
-CHECK-NEXT: ext_explicit_specialization_storage_class
CHECK-NEXT: ext_missing_whitespace_after_macro_name
CHECK-NEXT: ext_new_paren_array_nonconst
CHECK-NEXT: ext_plain_complex
|
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.
Should we add a test for this flag? Something around existing tests in clang/test/CXX/temp/temp.spec/temp.expl.spec/p2-20.cpp and clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp.
Oh, right, I did have a new test I meant to add, but that's a better place for it - so I've added that test coverage in b3facc9 (I'm out for the day, so if anyone happens to approve this and feel like pressing the "squash and merge" button that'd be swell, otherwise I'll get to it when I have a chance/tomorrow) |
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
Thank you!
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
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.
Minor nit: Probably don't need to explicitly define a diagnostic group, since it's only ever used for this diagnostic.
Not sure what the conventions are - but I rather like the normalization of having the explicit group, better chance of finding the group/reusing it maybe? So i'll leave it there for now, but if you feel strongly/other folks chime in - happy to change it in a follow-up commit. (& good to understand that explicit groups aren't necessary/the inline group definition is a valid option, though) |
…ializations (llvm#96699) With the recent fix for this situation in class members (llvm#93873) (for which the fixed code is invalid prior to this patch - making migrating code difficult as it must be in lock-step with the compiler migration, if building with -Werror) it'd be really useful to be able to disable this warning during the compiler migration/decouple the compiler migration from the source fixes. In theory this approach will regress the codebase to the previous non-member cases of this issue that were already being held back by the warning (as opposed to if we carved out the new cases into a separate warning from the existing cases) but I think this'll be so rare and the cleanup so simple, that the extra regressions of disabling the warning broadly won't be too much of a problem. (but if folks disagree, I'm open to making the warning more fine-grained)
…ializations (llvm#96699) With the recent fix for this situation in class members (llvm#93873) (for which the fixed code is invalid prior to this patch - making migrating code difficult as it must be in lock-step with the compiler migration, if building with -Werror) it'd be really useful to be able to disable this warning during the compiler migration/decouple the compiler migration from the source fixes. In theory this approach will regress the codebase to the previous non-member cases of this issue that were already being held back by the warning (as opposed to if we carved out the new cases into a separate warning from the existing cases) but I think this'll be so rare and the cleanup so simple, that the extra regressions of disabling the warning broadly won't be too much of a problem. (but if folks disagree, I'm open to making the warning more fine-grained)
cherry-picked: 9e1f1cf [email protected] Tue Jul 9 16:40:53 2024 -0400 [Clang][Sema] Handle class member access expressions with valid nested-name-specifiers that become invalid after lookup (llvm#98167) 584e431 [email protected] Wed Jul 3 12:12:53 2024 -0400 [Clang][Sema] Treat explicit specializations of static data member templates declared without 'static' as static data members when diagnosing uses of 'auto' (llvm#97425) a1bce0b [email protected] Thu Jun 27 08:17:40 2024 -0700 Clang: Add warning flag for storage class specifiers on explicit specializations (llvm#96699) f46d146 [email protected] Fri May 31 11:02:21 2024 -0700 [clang] require template arg list after template kw (llvm#80801) 033ec09 [email protected] Fri Dec 22 09:00:41 2023 +0800 [Clang][Sema] Fix Wswitch-default bad warning in template (llvm#76007) c281782 [email protected] Thu Dec 7 09:03:15 2023 +0800 [clang][Sema] Add -Wswitch-default warning option (llvm#73077) Change-Id: Ib2582201b01cc62c3bf62011347925556e8531f7
With the recent fix for this situation in class members (#93873) (for
which the fixed code is invalid prior to this patch - making migrating
code difficult as it must be in lock-step with the compiler migration,
if building with -Werror) it'd be really useful to be able to disable
this warning during the compiler migration/decouple the compiler
migration from the source fixes.
In theory this approach will regress the codebase to the previous
non-member cases of this issue that were already being held back by the
warning (as opposed to if we carved out the new cases into a separate
warning from the existing cases) but I think this'll be so rare and the
cleanup so simple, that the extra regressions of disabling the warning
broadly won't be too much of a problem. (but if folks disagree, I'm open
to making the warning more fine-grained)