Skip to content

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

Merged

Conversation

dwblaikie
Copy link
Collaborator

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)

…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)
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 25, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 25, 2024

@llvm/pr-subscribers-clang

Author: David Blaikie (dwblaikie)

Changes

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)


Full diff: https://github.com/llvm/llvm-project/pull/96699.diff

3 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticGroups.td (+4)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1-1)
  • (modified) clang/test/Misc/warning-flags.c (+1-2)
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

Copy link
Contributor

@alexfh alexfh left a 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.

@dwblaikie
Copy link
Collaborator Author

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)

@Endilll Endilll requested review from AaronBallman and Sirraide June 26, 2024 10:18
Copy link
Contributor

@alexfh alexfh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you!

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@sdkrystian sdkrystian left a 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.

@dwblaikie
Copy link
Collaborator Author

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)

@dwblaikie dwblaikie merged commit a1bce0b into llvm:main Jun 27, 2024
7 checks passed
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…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)
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
…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)
@dwblaikie dwblaikie deleted the clang_explicit_specialization_storage_warning branch July 31, 2024 17:37
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Oct 28, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants