-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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] emit -Wignored-qualifiers diagnostic for cv-qualified base classes #121419
Conversation
@llvm/pr-subscribers-libcxx @llvm/pr-subscribers-clang Author: Oleksandr T. (a-tarasyuk) ChangesFixes #55474 Full diff: https://github.com/llvm/llvm-project/pull/121419.diff 8 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b7da12bcf65818..659f0ebd97fc46 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -733,6 +733,7 @@ Improvements to Clang's diagnostics
scope.Unlock();
require(scope); // Warning! Requires mu1.
}
+- Clang now emits a ``-Wignored-qualifiers`` diagnostic when a base class includes cv-qualifiers (#GH55474).
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 330ae045616aba..ce8e9fc99dcbff 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -487,6 +487,10 @@ def err_noreturn_non_function : Error<
def warn_qual_return_type : Warning<
"'%0' type qualifier%s1 on return type %plural{1:has|:have}1 no effect">,
InGroup<IgnoredQualifiers>, DefaultIgnore;
+def warn_qual_base_type : Warning<
+ "'%0' qualifier%s1 on base class type %2 have no effect">,
+ InGroup<IgnoredQualifiers>;
+
def warn_deprecated_redundant_constexpr_static_def : Warning<
"out-of-line definition of constexpr static data member is redundant "
"in C++17 and is deprecated">,
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index c5a72cf812ebc9..df0a15fc44e3e3 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -2655,6 +2655,15 @@ CXXBaseSpecifier *Sema::CheckBaseSpecifier(CXXRecordDecl *Class,
return nullptr;
}
+ if (BaseType.hasQualifiers()) {
+ auto Quals =
+ BaseType.getQualifiers().getAsString(Context.getPrintingPolicy());
+ Diag(BaseLoc, diag::warn_qual_base_type)
+ << Quals << std::count(Quals.begin(), Quals.end(), ' ') + 1
+ << BaseType;
+ Diag(BaseLoc, diag::note_base_class_specified_here) << BaseType;
+ }
+
// For the MS ABI, propagate DLL attributes to base class templates.
if (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
Context.getTargetInfo().getTriple().isPS()) {
diff --git a/clang/test/CXX/drs/cwg4xx.cpp b/clang/test/CXX/drs/cwg4xx.cpp
index 825065840f1181..0c3ed2f4ebf534 100644
--- a/clang/test/CXX/drs/cwg4xx.cpp
+++ b/clang/test/CXX/drs/cwg4xx.cpp
@@ -1309,7 +1309,8 @@ namespace cwg484 { // cwg484: yes
}
CA::A() {}
- struct B : CA {
+ struct B : CA { // expected-warning {{'const' qualifier on base class type 'CA' (aka 'const cwg484::A') have no effect}} \
+ // expected-note {{base class 'CA' (aka 'const cwg484::A') specified here}}
B() : CA() {}
void f() { return CA::f(); }
};
diff --git a/clang/test/CXX/special/class.inhctor/elsewhere.cpp b/clang/test/CXX/special/class.inhctor/elsewhere.cpp
index f86f4b86faa7d7..079422fd99d98a 100644
--- a/clang/test/CXX/special/class.inhctor/elsewhere.cpp
+++ b/clang/test/CXX/special/class.inhctor/elsewhere.cpp
@@ -57,9 +57,10 @@ template<typename T> struct F : D<bool> {
F<bool> fb; // expected-note {{here}}
template<typename T>
-struct G : T {
+struct G : T { // expected-warning {{'const' qualifier on base class type 'const B1' have no effect}} \
+ // expected-note {{base class 'const B1' specified here}}
using T::T;
G(int &) : G(0) {}
};
G<B1> g(123);
-G<const B1> g2(123);
+G<const B1> g2(123); // expected-note {{in instantiation of template class 'G<const B1>' requested here}}
diff --git a/clang/test/Sema/GH70594.cpp b/clang/test/Sema/GH70594.cpp
index ce98e9b12b5cba..91ec31f6b053cb 100644
--- a/clang/test/Sema/GH70594.cpp
+++ b/clang/test/Sema/GH70594.cpp
@@ -1,12 +1,11 @@
// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
// RUN: %clang_cc1 -fsyntax-only -std=c++23 %s -verify
-// expected-no-diagnostics
-
struct A {};
using CA = const A;
-struct S1 : CA {
+struct S1 : CA { // expected-warning {{'const' qualifier on base class type 'CA' (aka 'const A') have no effect}} \
+ // expected-note {{base class 'CA' (aka 'const A') specified here}}
constexpr S1() : CA() {}
};
@@ -14,15 +13,17 @@ struct S2 : A {
constexpr S2() : CA() {}
};
-struct S3 : CA {
+struct S3 : CA { // expected-warning {{'const' qualifier on base class type 'CA' (aka 'const A') have no effect}} \
+ // expected-note {{base class 'CA' (aka 'const A') specified here}}
constexpr S3() : A() {}
};
struct Int {};
template <class _Hp>
-struct __tuple_leaf : _Hp {
+struct __tuple_leaf : _Hp { // expected-warning {{'const' qualifier on base class type 'const Int' have no effect}} \
+ // expected-note {{base class 'const Int' specified here}}
constexpr __tuple_leaf() : _Hp() {}
};
-constexpr __tuple_leaf<const Int> t;
+constexpr __tuple_leaf<const Int> t; // expected-note {{in instantiation of template class '__tuple_leaf<const Int>' requested here}}
diff --git a/clang/test/SemaCXX/class-base-member-init.cpp b/clang/test/SemaCXX/class-base-member-init.cpp
index a6bb4410a81c84..e46c79a2b68b9c 100644
--- a/clang/test/SemaCXX/class-base-member-init.cpp
+++ b/clang/test/SemaCXX/class-base-member-init.cpp
@@ -103,7 +103,8 @@ namespace PR16596 {
class A { public: virtual ~A(); };
typedef const A Foo;
void Apply(Foo processor);
- struct Bar : public Foo {};
+ struct Bar : public Foo {}; // expected-warning {{'const' qualifier on base class type 'Foo' (aka 'const PR16596::A') have no effect}}\
+ // expected-note {{base class 'Foo' (aka 'const PR16596::A') specified here}}
void Fetch() {
Apply(Bar());
}
diff --git a/clang/test/SemaCXX/warn-base-type-qualifiers.cpp b/clang/test/SemaCXX/warn-base-type-qualifiers.cpp
new file mode 100644
index 00000000000000..5074140700cc36
--- /dev/null
+++ b/clang/test/SemaCXX/warn-base-type-qualifiers.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers -verify
+
+class A { };
+
+typedef const A A_Const;
+class B : public A_Const { }; // expected-warning {{'const' qualifier on base class type 'A_Const' (aka 'const A') have no effect}} \
+ // expected-note {{base class 'A_Const' (aka 'const A') specified here}}
+
+typedef const volatile A A_Const_Volatile;
+class C : public A_Const_Volatile { }; // expected-warning {{'const volatile' qualifiers on base class type 'A_Const_Volatile' (aka 'const volatile A') have no effect}} \
+ // expected-note {{base class 'A_Const_Volatile' (aka 'const volatile A') specified here}}
|
f3b5fc2
to
902d4f0
Compare
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.
I have a bit of a concern that the template cases are going to be REALLY noisy, but we can deal with restricting individual use cases to a separate warning-group in the future if need-be.
@erichkeane It looks like that's why I have two failed tests from |
Yep, that looks to be exactly the case! I'd be interested to see if @ldionne would be willing to accept a |
@erichkeane @ldionne should the |
This is entirely a @ldionne question, though I see in github that he's on vacation at the moment, so we'll have to wait until he returns. |
Thanks for the ping. I've returned but Github doesn't clear the vacation status automatically - fixed now. This seems reasonable to me, we should "fix" libc++. I think this should do it: diff --git a/libcxx/include/tuple b/libcxx/include/tuple
index aca14ba408d3..90257e9bae0a 100644
--- a/libcxx/include/tuple
+++ b/libcxx/include/tuple
@@ -257,6 +257,7 @@ template <class... Types>
# include <__type_traits/maybe_const.h>
# include <__type_traits/nat.h>
# include <__type_traits/negation.h>
+# include <__type_traits/remove_cv.h>
# include <__type_traits/remove_cvref.h>
# include <__type_traits/remove_reference.h>
# include <__type_traits/unwrap_ref.h>
@@ -535,7 +536,7 @@ __memberwise_forward_assign(_Dest& __dest, _Source&& __source, __tuple_types<_Up
template <class... _Tp>
class _LIBCPP_TEMPLATE_VIS tuple {
- typedef __tuple_impl<typename __make_tuple_indices<sizeof...(_Tp)>::type, _Tp...> _BaseT;
+ typedef __tuple_impl<typename __make_tuple_indices<sizeof...(_Tp)>::type, __remove_cv_t<_Tp>...> _BaseT;
_BaseT __base_;
|
ce918f9
to
018d4fc
Compare
As far as the CFE is concerned, we're OK with this. Though note, I anticipate quite a bit of push back as libraries discover this warning, though most should change their code instead, so I don't anticipate a good reason to revert. |
018d4fc
to
3aa2e5f
Compare
@ldionne, @philnik777, other contributors: can you confirm that the small libcxx change in this patch is acceptable? Thanks! |
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.
This makes me wonder how many other instances like this there are in the wild in generic code, but I guess compressed_pair-style classes aren't that ubiquitous. LGTM.
TBH, I'm actually shocked that there is only 1 Libc++ violation of this warning. I was honestly quite positive that we'd see about a million there, plus another trillion in user code, as no one has ever really cared about this in the past. I'm finding myself to be mildly optimistic here thanks to you guys only having 1 violation :) Thanks for the review! @a-tarasyuk : feel free to merge at your convenience. |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/12116 Here is the relevant piece of the build log for the reference
|
Could we make bootstrapping warning-free before adding warnings? |
…124193) This resolves the `-Wignored-qualifiers` warning introduced by the new warnign in #121419. First caught in buildbot `ppc64le-lld-multistage-test` https://lab.llvm.org/buildbot/#/builders/168/builds/7756 --------- Co-authored-by: Henry Jiang <[email protected]>
…nternal.h (#124193) This resolves the `-Wignored-qualifiers` warning introduced by the new warnign in llvm/llvm-project#121419. First caught in buildbot `ppc64le-lld-multistage-test` https://lab.llvm.org/buildbot/#/builders/168/builds/7756 --------- Co-authored-by: Henry Jiang <[email protected]>
llvm/llvm-project#121419 added -Wignored-qualifiers diagnostic for cv-qualified base classes. This patch removes const from MockEthernetImpl to fix the -Wignored-qualifiers error. Bug: 391704289 Change-Id: I0d5bce970d6e91febe7a5f775b35b0b2e1383245 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1192542 Commit-Queue: Auto-Submit <[email protected]> Reviewed-by: Bruno Dal Bo <[email protected]> Fuchsia-Auto-Submit: Gulfem Savrun Yeniceri <[email protected]>
llvm/llvm-project#121419 added -Wignored-qualifiers diagnostic for cv-qualified base classes. This patch removes const from MockNetworkDeviceIfc to fix the -Wignored-qualifiers error similar to https://fxrev.dev/1192542. Bug: 391704289 Change-Id: I01fe73aa25850fa21d3ac10a14e894aed51e5419 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1194243 Fuchsia-Auto-Submit: Gulfem Savrun Yeniceri <[email protected]> Reviewed-by: Bruno Dal Bo <[email protected]> Commit-Queue: Auto-Submit <[email protected]>
llvm/llvm-project#121419 added -Wignored-qualifiers diagnostic for cv-qualified base classes. This patch removes const from MockEthernetImpl to fix the -Wignored-qualifiers error. Original-Bug: 391704289 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1192542 Original-Revision: 626de676a7b41c8be75aeb1fb7030735aa3f12d5 GitOrigin-RevId: fda0189e659871a285e713d06da62c0eaf8e7764 Change-Id: Iebcf36703ad221a3099d618e5ed14d9b048576c2
llvm/llvm-project#121419 added -Wignored-qualifiers diagnostic for cv-qualified base classes. This patch removes const from MockNetworkDeviceIfc to fix the -Wignored-qualifiers error similar to https://fxrev.dev/1192542. Original-Bug: 391704289 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1194243 Original-Revision: 49dbcd7dd9a6aedc8294cc2cf816727e25706294 GitOrigin-RevId: 3785f90762ae439b3034af9a0a072de315e20664 Change-Id: Ia059b5d0303ca0ed94c40ac4dcc1e34533807fbb
Fixes #55474