[Diagnostic] Split 'qualifier on reference type has no effect' out into a new flag

[lunasorcery]

Summary

This introduces a new flag ignored-reference-qualifiers for the existing "'A' qualifier on reference type B has no effect" diagnostic, as a child of ignored-qualifiers.

Rationale:
This particular diagnostic is enabled by default, but other parts of ignored-qualifiers are not. Anecdotally, a user may encounter this diagnostic in the wild, and, seeing it to be valuable, might try to raise it to error with -Werror=ignored-qualifiers, whereupon the other diagnostics the flag covers will also be raised, to the user's surprise and confusion. By splitting this diagnostic out into a separate flag, and marking it as a child of ignored-qualifiers, we allow the user more granular control of the diagnostics they care about, while maintaining backwards compatibility with existing build scripts.

[aaron.ballman]

Thank you for this! The changes LGTM as far as they go, but I'd appreciate some test coverage that shows the new diagnostic can be disabled.

[lunasorcery]

Sure thing - would the suite in /clang/test/SemaCXX/ be the right place to put that?

[aaron.ballman]

In D107125#2917668, @lunasorcery wrote:

Sure thing - would the suite in /clang/test/SemaCXX/ be the right place to put that?

Yeah, that'd be a great place for it. Thank you!

[lunasorcery]

How does this look? I've got a 'control' test to demonstrate the diagnostic as a child of ignored-qualifiers, as well as a test to demonstrate this diagnostic can be disabled in isolation.

[aaron.ballman]

In D107125#2920989, @lunasorcery wrote:

How does this look? I've got a 'control' test to demonstrate the diagnostic as a child of ignored-qualifiers, as well as a test to demonstrate this diagnostic can be disabled in isolation.

The test contents are great, but I had a suggestion for combining the test files.

clang/test/SemaCXX/ignored-reference-qualifiers-disabled.cpp

1–20

Given that the test file contents are the same except for expected diagnostics, I would recommend combining them into a single test file with two // RUN lines. Something like this (untested, but hopefully it gives you the right idea):

// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers -Wno-ignored-reference-qualifiers -verify=both
// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers -verify=both,qual

const int scalar_c(); // both-warning{{'const' type qualifier on return type has no effect}}
volatile int scalar_v(); // both-warning{{'volatile' type qualifier on return type has no effect}}
const volatile int scalar_cv(); // both-warning{{'const volatile' type qualifiers on return type have no effect}}

typedef int& IntRef;

const IntRef ref_c(); // qual-warning{{'const' qualifier on reference type 'IntRef' (aka 'int &') has no effect}}
volatile IntRef ref_v(); // qual-warning{{'volatile' qualifier on reference type 'IntRef' (aka 'int &') has no effect}}
const volatile IntRef ref_cv(); // qual-warning{{'const' qualifier on reference type 'IntRef' (aka 'int &') has no effect}} \
                                qual-warning{{'volatile' qualifier on reference type 'IntRef' (aka 'int &') has no effect}}

template<typename T>
class container {
	using value_type = T;
	using reference  = value_type&;
	reference get();
	const reference get() const; // qual-warning{{'const' qualifier on reference type 'container::reference' (aka 'T &') has no effect}}
};

[lunasorcery]

Ah, that's much cleaner! I hadn't realized the test framework could do that.
I've updated the diff accordingly - your example works perfectly, so I've taken that and ditched the now-unnecessary other test.

[aaron.ballman]

LGTM, thank you for the patch! Do you need me to commit it on your behalf? If so, what name and email address would you like me to use for patch attribution in git?

[lunasorcery]

Yes, that would be "Luna Kirkby <[e-mail]>"

[aaron.ballman]

I've commit on your behalf in 6385abd0c4490e0516cb31c0b86c0fbcc052f815, thank you for the patch!

clang/include/clang/Basic/DiagnosticGroups.td

 def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
 def ExcessInitializers : DiagGroup<"excess-initializers">;
 def ExpansionToDefined : DiagGroup<"expansion-to-defined">;
 def FlagEnum : DiagGroup<"flag-enum">;
 def IncrementBool : DiagGroup<"increment-bool", [DeprecatedIncrementBool]>;
 def InfiniteRecursion : DiagGroup<"infinite-recursion">;
 def PureVirtualCallFromCtorDtor: DiagGroup<"call-to-pure-virtual-from-ctor-dtor">;
 def GNUImaginaryConstant : DiagGroup<"gnu-imaginary-constant">;
-def IgnoredQualifiers : DiagGroup<"ignored-qualifiers">;
+def IgnoredReferenceQualifiers : DiagGroup<"ignored-reference-qualifiers">;
+def IgnoredQualifiers : DiagGroup<"ignored-qualifiers", [IgnoredReferenceQualifiers]>;
 def : DiagGroup<"import">;
 def GNUIncludeNext : DiagGroup<"gnu-include-next">;
 def IncompatibleMSStruct : DiagGroup<"incompatible-ms-struct">;
 def IncompatiblePointerTypesDiscardsQualifiers
   : DiagGroup<"incompatible-pointer-types-discards-qualifiers">;
 def IncompatibleFunctionPointerTypes
   : DiagGroup<"incompatible-function-pointer-types">;
 def IncompatiblePointerTypes

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_typecheck_negative_array_size : Error<"array size is negative">;
 def warn_typecheck_function_qualifiers_ignored : Warning<
   "'%0' qualifier on function type %1 has no effect">,
   InGroup<IgnoredQualifiers>;
 def warn_typecheck_function_qualifiers_unspecified : Warning<
   "'%0' qualifier on function type %1 has unspecified behavior">;
 def warn_typecheck_reference_qualifiers : Warning<
   "'%0' qualifier on reference type %1 has no effect">,
-  InGroup<IgnoredQualifiers>;
+  InGroup<IgnoredReferenceQualifiers>;
 def err_typecheck_invalid_restrict_not_pointer : Error<
   "restrict requires a pointer or reference (%0 is invalid)">;
 def err_typecheck_invalid_restrict_not_pointer_noarg : Error<
   "restrict requires a pointer or reference">;
 def err_typecheck_invalid_restrict_invalid_pointee : Error<
   "pointer to function type %0 may not be 'restrict' qualified">;
 def ext_typecheck_zero_array_size : Extension<
   "zero size arrays are an extension">, InGroup<ZeroLengthArray>;

clang/test/SemaCXX/ignored-reference-qualifiers-disabled.cpp

+// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers -Wno-ignored-reference-qualifiers -verify
+
+const int scalar_c(); // expected-warning{{'const' type qualifier on return type has no effect}}
+volatile int scalar_v(); // expected-warning{{'volatile' type qualifier on return type has no effect}}
+const volatile int scalar_cv(); // expected-warning{{'const volatile' type qualifiers on return type have no effect}}
+
+// no warnings expected below here, as we've disabled ignored-reference-qualifiers.
+typedef int& IntRef;
+
+const IntRef ref_c();
+volatile IntRef ref_v();
+const volatile IntRef ref_cv();
+
+template<typename T>
+class container {
+  using value_type = T;
+  using reference  = value_type&;
+  reference get();
+  const reference get() const;
+};

clang/test/SemaCXX/ignored-reference-qualifiers.cpp

+// RUN: %clang_cc1 %s -std=c++11 -Wignored-qualifiers -verify
+
+const int scalar_c(); // expected-warning{{'const' type qualifier on return type has no effect}}
+volatile int scalar_v(); // expected-warning{{'volatile' type qualifier on return type has no effect}}
+const volatile int scalar_cv(); // expected-warning{{'const volatile' type qualifiers on return type have no effect}}
+
+typedef int& IntRef;
+
+const IntRef ref_c(); // expected-warning{{'const' qualifier on reference type 'IntRef' (aka 'int &') has no effect}}
+volatile IntRef ref_v(); // expected-warning{{'volatile' qualifier on reference type 'IntRef' (aka 'int &') has no effect}}
+const volatile IntRef ref_cv(); /* expected-warning{{'const' qualifier on reference type 'IntRef' (aka 'int &') has no effect}} \
+                                expected-warning{{'volatile' qualifier on reference type 'IntRef' (aka 'int &') has no effect}} */
+
+template<typename T>
+class container {
+  using value_type = T;
+  using reference  = value_type&;
+  reference get();
+  const reference get() const; // expected-warning{{'const' qualifier on reference type 'container::reference' (aka 'T &') has no effect}}
+};