[Clang] add emit -Wignored-base-class-qualifiers diagnostic for cv-qualified base classes #132116

lyledean1 · 2025-03-19T22:15:59Z

Split diagnosing base class qualifiers from the -Wignored-Qualifiers diagnostic group into a new -Wignored-base-class-qualifiers diagnostic group (which is grouped under -Wignored-qualifiers).

Fixes: #131935

github-actions · 2025-03-19T22:16:22Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

lyledean1 · 2025-03-19T22:20:34Z

@erichkeane , I've added what I think is the correct approach for #131935 by adding a new group to DiagnosticGroups.td

Not sure on what the best name is for the new group, so added "IgnoredCVQualifiers" for the time being

Let me know if this is going in the right direction - feedback would be appreciated and set as a draft PR

erichkeane

This I think ends up being the 'minimum' patch here, but see my suggestion on the bug:

I also believe there is value to only diagnosing this on EXPLICIT qualifiers (or a separate, additional warning to do so), not ones that get picked up from the template.

The more I think about it, the more I think that we should modify the warning to only happen on EXPLICIT qualifiers. So:

struct Base{};
using CBase = const Base;

struct D1 : const Base {}; // DOES warn
struct D2 : CBase{}; // a DIFFERENT warning (same wording perhaps?) under a different group.
template<typename T> Templ: const T{};
Templ<Base> t; // Causes same warning as D1.
template<typename T> Templ2 : T{};  
Templ2<CBase> t; // Causes same warning as D2.

If you search for where that diagnostic is emitted, you should see that. You might have to do some debugging to figure out how to differentiate the cases though.

erichkeane · 2025-03-20T12:55:50Z

clang/docs/ReleaseNotes.rst

@@ -284,6 +284,8 @@ Improvements to Clang's diagnostics

 - Improve the ``-Wundefined-func-template`` warning when a function template is not instantiated due to being unreachable in modules.

+- Clang now emits a ``-Wignored-cv-qualifiers`` diagnostic when a base class includes cv-qualifiers ([toadd]).


what is [toadd]?

on the original PR release note i saw a reference to the feature number i.e (#GH55474) and wasn't sure if i needed to add it here - removed it

erichkeane · 2025-03-20T12:56:51Z

clang/include/clang/Basic/DiagnosticGroups.td

@@ -497,6 +497,7 @@ def GNUImaginaryConstant : DiagGroup<"gnu-imaginary-constant">;
 def IgnoredGCH : DiagGroup<"ignored-gch">;
 def IgnoredReferenceQualifiers : DiagGroup<"ignored-reference-qualifiers">;
 def IgnoredQualifiers : DiagGroup<"ignored-qualifiers", [IgnoredReferenceQualifiers]>;
+def IgnoredCVQualifiers : DiagGroup<"ignored-cv-qualifiers", [IgnoredReferenceQualifiers]>;


I'd probably choose a better name for this group. I think it should be clear that this is for base classes, it isn't cv qualifiers, it is base classes.

So something like ignored-base-class-qualifiers.

I've updated this, so all references will be to ignored-base-class-qualifiers now

AaronBallman · 2025-03-20T14:11:24Z

Please also be sure to add enough information to the PR summary to explain why the changes are needed (the summary becomes the commit message).

MagentaTreehouse · 2025-03-20T16:08:45Z

The more I think about it, the more I think that we should modify the warning to only happen on EXPLICIT qualifiers. So:

struct Base{};
using CBase = const Base;

struct D1 : const Base {}; // DOES warn
struct D2 : CBase{}; // a DIFFERENT warning (same wording perhaps?) under a different group.
template<typename T> Templ: const T{};
Templ<Base> t; // Causes same warning as D1.
template<typename T> Templ2 : T{};  
Templ2<CBase> t; // Causes same warning as D2.

Adding qualifiers directly in base specifiers is invalid (and all major compilers correctly reject this). See grammar in [class.derived.general]/1. Qualifiers can indirectly come from decltype, template parameters, type aliases and so on.

erichkeane · 2025-03-20T16:13:32Z

The more I think about it, the more I think that we should modify the warning to only happen on EXPLICIT qualifiers. So:
struct Base{};
using CBase = const Base;

struct D1 : const Base {}; // DOES warn
struct D2 : CBase{}; // a DIFFERENT warning (same wording perhaps?) under a different group.
template<typename T> Templ: const T{};
Templ<Base> t; // Causes same warning as D1.
template<typename T> Templ2 : T{};  
Templ2<CBase> t; // Causes same warning as D2.
Adding qualifiers directly in base specifiers is invalid (and all major compilers correctly reject this). See grammar in [class.derived.general]/1. Qualifiers can indirectly come from decltype, template parameters, type aliases and so on.

Ah, cool. I didn't realize we rejected that already. Then I think adding this to a new flag is sufficient, so probably OK after Aaron + My comments are addressed.

lyledean1 · 2025-03-20T23:18:43Z

I've addressed the comments @erichkeane , so its now -Wignored-base-class-qualifiers

@AaronBallman , let me know if that summary isn't sufficient

llvmbot · 2025-03-20T23:19:20Z

@llvm/pr-subscribers-clang

Author: Lyle Dean (lyledean1)

Changes

This splits base class qualifiers from ignored qualifiers -Wignored-qualifiers to its own ignore base class qualifiers group -Wignored-base-class-qualifiers to reduce noise as it is not enabled by -Wextra

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

4 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/include/clang/Basic/DiagnosticGroups.td (+1)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1-1)
(modified) clang/test/SemaCXX/warn-base-type-qualifiers.cpp (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9b0680c68b83a..82afde2a8d229 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -284,6 +284,8 @@ Improvements to Clang's diagnostics
 
 - Improve the ``-Wundefined-func-template`` warning when a function template is not instantiated due to being unreachable in modules.
 
+- Clang now emits a ``-Wignored-base-class-qualifiers`` diagnostic when a base class includes cv-qualifiers.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index b9f08d96151c9..a8b9f9c9a6971 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -497,6 +497,7 @@ def GNUImaginaryConstant : DiagGroup<"gnu-imaginary-constant">;
 def IgnoredGCH : DiagGroup<"ignored-gch">;
 def IgnoredReferenceQualifiers : DiagGroup<"ignored-reference-qualifiers">;
 def IgnoredQualifiers : DiagGroup<"ignored-qualifiers", [IgnoredReferenceQualifiers]>;
+def IgnoredBaseClassQualifiers : DiagGroup<"ignored-base-class-qualifiers", [IgnoredReferenceQualifiers]>;
 def : DiagGroup<"import">;
 def GNUIncludeNext : DiagGroup<"gnu-include-next">;
 def IncompatibleMSStruct : DiagGroup<"incompatible-ms-struct">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 1536a3b8c920a..720570fed2af5 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -492,7 +492,7 @@ def warn_qual_return_type : Warning<
   InGroup<IgnoredQualifiers>, DefaultIgnore;
 def warn_qual_base_type : Warning<
   "'%0' qualifier%s1 on base class type %2 %plural{1:has|:have}1 no effect">,
-  InGroup<IgnoredQualifiers>, DefaultIgnore;
+  InGroup<IgnoredBaseClassQualifiers>, DefaultIgnore;
 
 def warn_deprecated_redundant_constexpr_static_def : Warning<
   "out-of-line definition of constexpr static data member is redundant "
diff --git a/clang/test/SemaCXX/warn-base-type-qualifiers.cpp b/clang/test/SemaCXX/warn-base-type-qualifiers.cpp
index 7c775a552dd88..2879c8dda5e97 100644
--- a/clang/test/SemaCXX/warn-base-type-qualifiers.cpp
+++ b/clang/test/SemaCXX/warn-base-type-qualifiers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers -verify
+// RUN: %clang_cc1 %s -std=c++11 -Wignored-base-class-qualifiers -verify
 
 template <typename T> struct add_const {
   using type = const T;

erichkeane

I'm happy when Aaron is happy with commit message/etc. THOUGH we should do Fixes: #<BUG NUMBER> in it so it auto-closes the bug.

I suspect we'll have to merge this for you, so if you notice CI has passed, feel free to ping us and we'll do it.

MagentaTreehouse · 2025-03-21T14:13:53Z

I suggest backporting to the 20 release. Wdyt? (If we decide so, the release notes would go into 20 instead of main)

lyledean1 · 2025-03-28T19:52:35Z

Happy to adjust this @MagentaTreehouse for LLVM 20 - what would that require?

@erichkeane is this ok to merge (assuming I make the changes for the above - the CI has passed)

clang/docs/ReleaseNotes.rst

clang/include/clang/Basic/DiagnosticGroups.td

clang/test/SemaCXX/warn-base-type-qualifiers.cpp

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

lyledean1 · 2025-03-31T21:49:15Z

Thanks for the review @AaronBallman , I've updated the PR from your comments and the CI has passed - is this good to merge now?

MagentaTreehouse · 2025-04-01T00:00:06Z

Happy to adjust this @MagentaTreehouse for LLVM 20 - what would that require?

@lyledean1 I think the process is that the release notes are left unchanged in this PR; after it is merged we add this to LLVM 20.X Release milestone, open a backport PR by cherry-picking the commit (Example), and add changes to the release notes there.

AaronBallman

LGTM!

AaronBallman · 2025-04-01T14:46:20Z

Happy to adjust this @MagentaTreehouse for LLVM 20 - what would that require?

@lyledean1 I think the process is that the release notes are left unchanged in this PR; after it is merged we add this to LLVM 20.X Release milestone, open a backport PR by cherry-picking the commit (Example), and add changes to the release notes there.

I don't think this qualifies for backporting. It's not fixing a bug, and it's technically an ABI break because we generate enumerators for the diagnostic IDs and this will shift the numbers around.

AaronBallman · 2025-04-01T14:47:19Z

Once you resolve the merge conflicts so precommit CI comes back green, I can land the changes

MagentaTreehouse · 2025-04-01T16:18:15Z

clang/docs/ReleaseNotes.rst

@@ -298,6 +298,8 @@ Improvements to Clang's diagnostics

 - Improve the ``-Wundefined-func-template`` warning when a function template is not instantiated due to being unreachable in modules.

+- Split diagnosing base class qualifiers from the ``-Wignored-Qualifiers`` diagnostic group into a new ``-Wignored-base-class-qualifiers`` diagnostic group (which is grouped under ``-Wignored-qualifiers``). Fixes #GH131935.


The "is grouped under" relationship is the wrong way around. Should it be "controls"?

I think the wording is sufficient. No need to change IMO.

erichkeane · 2025-04-01T17:03:23Z

It isn't clear what the merge conflicts are (github won't tell me for some reason?) but those need fixing before we can merge this.

lyledean1 · 2025-04-01T22:39:04Z

It isn't clear what the merge conflicts are (github won't tell me for some reason?) but those need fixing before we can merge this.

It was on the release notes, that's fixed now

lyledean1 · 2025-04-01T22:48:56Z

CI seems to be failing with the message no such option: --break-system-packages from pip for buildkite/github-pull-requests/linux-linux-x64 and buildkite/github-pull-requests

But haven't changed anything around this?

AaronBallman · 2025-04-02T11:31:25Z

Precommit CI failure on Linux appears to be unrelated, so landing the changes (I'll watch the bots in case it was related somehow).

github-actions · 2025-04-02T11:32:08Z

@lyledean1 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

lyledean1 added 2 commits March 19, 2025 22:13

Add Ignored CV Qualifiers

4947bb5

remove build

4c932ce

Update ReleaseNotes.rst

1d8b89d

lyledean1 mentioned this pull request Mar 19, 2025

[Clang][Diagnostics] Consider splitting warning on cv-qualified base classes into a separate group #131935

Closed

erichkeane requested a review from AaronBallman March 20, 2025 12:55

erichkeane reviewed Mar 20, 2025

View reviewed changes

lyledean1 added 4 commits March 20, 2025 22:49

Update to base-class-qualifiers name

01e74d4

Update to base-class-qualifiers name

925c19e

Update to base-class-qualifiers name

97f49f9

Remove toadd comment

445019b

lyledean1 changed the title ~~[Clang] add emit -Wignored-cv-qualifiers diagnostic for cv-qualified base classes~~ [Clang] add emit -Wignored-base-class-qualifiers diagnostic for cv-qualified base classes Mar 20, 2025

lyledean1 marked this pull request as ready for review March 20, 2025 23:18

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 20, 2025

erichkeane approved these changes Mar 21, 2025

View reviewed changes

Merge branch 'main' into add-ignored-cv-qualifiers

30a2ab8

AaronBallman reviewed Mar 31, 2025

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

clang/include/clang/Basic/DiagnosticGroups.td Outdated Show resolved Hide resolved

clang/test/SemaCXX/warn-base-type-qualifiers.cpp Show resolved Hide resolved

lyledean1 and others added 3 commits March 31, 2025 21:09

Update clang/docs/ReleaseNotes.rst

0675b4b

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Update clang/include/clang/Basic/DiagnosticGroups.td

5efd8b2

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Update warn base type qualifier tests

bd475a5

AaronBallman approved these changes Apr 1, 2025

View reviewed changes

MagentaTreehouse reviewed Apr 1, 2025

View reviewed changes

Merge branch 'main' into add-ignored-cv-qualifiers

d3d3a91

Update ReleaseNotes.rst

36014e2

Update ReleaseNotes.rst

0d7741a

AaronBallman merged commit a0b75b9 into llvm:main Apr 2, 2025
5 of 7 checks passed

lyledean1 deleted the add-ignored-cv-qualifiers branch April 2, 2025 20:19

		@@ -284,6 +284,8 @@ Improvements to Clang's diagnostics

		- Improve the ``-Wundefined-func-template`` warning when a function template is not instantiated due to being unreachable in modules.

		- Clang now emits a ``-Wignored-cv-qualifiers`` diagnostic when a base class includes cv-qualifiers ([toadd]).

		@@ -298,6 +298,8 @@ Improvements to Clang's diagnostics

		- Improve the ``-Wundefined-func-template`` warning when a function template is not instantiated due to being unreachable in modules.

		- Split diagnosing base class qualifiers from the ``-Wignored-Qualifiers`` diagnostic group into a new ``-Wignored-base-class-qualifiers`` diagnostic group (which is grouped under ``-Wignored-qualifiers``). Fixes #GH131935.

[Clang] add emit -Wignored-base-class-qualifiers diagnostic for cv-qualified base classes #132116

[Clang] add emit -Wignored-base-class-qualifiers diagnostic for cv-qualified base classes #132116

Uh oh!

Conversation

lyledean1 commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 19, 2025

Uh oh!

lyledean1 commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

lyledean1 Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

lyledean1 Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Mar 20, 2025

Uh oh!

MagentaTreehouse commented Mar 20, 2025

Uh oh!

erichkeane commented Mar 20, 2025

Uh oh!

lyledean1 commented Mar 20, 2025

Uh oh!

llvmbot commented Mar 20, 2025

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

MagentaTreehouse commented Mar 21, 2025

Uh oh!

lyledean1 commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyledean1 commented Mar 31, 2025

Uh oh!

MagentaTreehouse commented Apr 1, 2025

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Apr 1, 2025

Uh oh!

AaronBallman commented Apr 1, 2025

Uh oh!

MagentaTreehouse Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Apr 1, 2025

Uh oh!

lyledean1 commented Apr 1, 2025

Uh oh!

lyledean1 commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Apr 2, 2025

Uh oh!

Uh oh!

github-actions bot commented Apr 2, 2025

Uh oh!

Uh oh!

lyledean1 commented Mar 19, 2025 •

edited

Loading

lyledean1 commented Mar 19, 2025 •

edited

Loading

lyledean1 commented Apr 1, 2025 •

edited

Loading