This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by lunasorcery on Jul 29 2021, 6:12 PM.

Details

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.

Diff Detail

Event Timeline

lunasorcery requested review of this revision.Jul 29 2021, 6:12 PM
lunasorcery created this revision.
lunasorcery edited the summary of this revision. (Show Details)Jul 29 2021, 6:15 PM

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.

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

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!

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.

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
2–21

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}}
};

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 accepted this revision.Aug 5 2021, 4:26 AM

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?

This revision is now accepted and ready to land.Aug 5 2021, 4:26 AM
lunasorcery marked an inline comment as done.Aug 5 2021, 2:41 PM

Yes, that would be "Luna Kirkby <llvm@moonbase.lgbt>"

aaron.ballman closed this revision.Aug 6 2021, 4:09 AM

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