Page MenuHomePhabricator

[clang] Add -Wsuggest-override
ClosedPublic

Authored by logan-5 on Jun 28 2020, 1:13 PM.

Details

Summary

This patch adds -Wsuggest-override, which allows for more aggressive enforcement of modern C++ best practices, as well as better compatibility with gcc, which has had its own -Wsuggest-override since version 5.1.

Clang already has -Winconsistent-missing-override, which only warns in the case where there is at least one function already marked override in a class. This warning strengthens that warning by suggesting the override keyword regardless of whether it is already present anywhere.

The text between suggest-override and inconsistent-missing-override is now shared, using TextSubstitution for the entire diagnostic text.

Diff Detail

Event Timeline

logan-5 created this revision.Jun 28 2020, 1:13 PM
logan-5 updated this revision to Diff 273986.Jun 28 2020, 6:50 PM

Ran clang-format over the diff.

logan-5 updated this revision to Diff 273991.Jun 28 2020, 8:14 PM

Don't warn for destructors by default. This makes -Wsuggest-[destructor-]override more consistent with the behavior of -Winconsistent-missing-[destructor-]override, as well as gcc.

e4lam added a subscriber: e4lam.Jun 29 2020, 7:19 PM

Generally Clang tries to avoid stylistic warnings like this (& they are left to be addressed by tools like clang-tidy), hence why -Winconsistent-missing-override was implemented that way (the presence of at least one "override" being a signal the user intended to use override and missed some, rather than that maybe the user hadn't intended to use it at all (because they were compiling their code with multiple versions, etc)). (but I wouldn't outright rule this out - just mentioning the general principle here, perhaps @rsmith or others have other ideas.

(the presence of at least one "override" being a signal the user intended to use override and missed some [...]

I'm in favor of -Wsuggest-override, and would definitely use it if it existed. The problem I see with -Wmissing-override, as it's been implemented, is that it uses the wrong granularity for "intent": it looks only across the methods of a single class, rather than across all the classes of a single header, or across a single translation unit, or across my entire codebase. In real life, I always want to look across my entire codebase (excluding system headers). If any class in my project uses override, I want Clang to take that as a clear declaration of intent to use override throughout; I don't want Clang treating class A differently from class B. But of course Clang can't look at my whole codebase simultaneously. So the next best thing is to give the user a simple way to "preload the intent flag": to say "As soon as you start processing any class, please act as if intent has been declared for that class." Adding -Wsuggest-override to my build line seems like a perfect way to implement that "preload" facility.

clang/lib/Sema/SemaDeclCXX.cpp
3075

These linebreaks are super unfortunate. Could they be improved by doing it like this?

auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
  unsigned DiagID = isa<CXXDestructorDecl>(MD) ? DiagDtor : DiagFn;
  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
};
if (Inconsistent)
  EmitDiag(diag::warn_inconsistent_destructor_marked_not_override_overriding,
           diag::warn_inconsistent_function_marked_not_override_overriding);
else
  EmitDiag(diag::warn_suggest_destructor_marked_not_override_overriding
           diag::warn_suggest_function_marked_not_override_overriding);
6763

Can you s/InconsistentOverrideControl/HasInconsistentOverrideControl/ without causing bad linebreaks?

clang/test/SemaCXX/warn-suggest-destructor-override
7

Surely this doesn't compile?!

(the presence of at least one "override" being a signal the user intended to use override and missed some [...]

I'm in favor of -Wsuggest-override, and would definitely use it if it existed.

I've no doubt a non-trivial number of folks would - we'd probably enable it in LLVM itself.

The problem I see with -Wmissing-override, as it's been implemented, is that it uses the wrong granularity for "intent": it looks only across the methods of a single class, rather than across all the classes of a single header, or across a single translation unit, or across my entire codebase. In real life, I always want to look across my entire codebase (excluding system headers). If any class in my project uses override, I want Clang to take that as a clear declaration of intent to use override throughout; I don't want Clang treating class A differently from class B. But of course Clang can't look at my whole codebase simultaneously.

Right - Clang's doing its best (well, debatable - that's what we're debating) with what it's got.

So the next best thing is to give the user a simple way to "preload the intent flag": to say "As soon as you start processing any class, please act as if intent has been declared for that class." Adding -Wsuggest-override to my build line seems like a perfect way to implement that "preload" facility.

The issue is that such a warning then needs to be off by default, because we can't assume the user's intent. And Clang's historically been fairly averse to off-by-default warnings due to low user-penetration (not zero, like I said, I imagine LLVM itself would use such a warning, were it implemented) & so concerns about the cost/benefit tradeoff of the added complexity (source code and runtime) of the feature.

logan-5 updated this revision to Diff 275928.Jul 7 2020, 12:21 AM
logan-5 marked 6 inline comments as done.

Addressed some feedback.

Glad this is generating some discussion. For my $0.02, I would also (obviously) love to be able to enable this warning on all the codebases I work on, and this patch was born out of a discussion on the C++ Slack with another user who had found this warning very useful in GCC and was wondering why Clang didn't have it yet.

The issue is that such a warning then needs to be off by default, because we can't assume the user's intent. And Clang's historically been fairly averse to off-by-default warnings due to low user-penetration (not zero, like I said, I imagine LLVM itself would use such a warning, were it implemented) & so concerns about the cost/benefit tradeoff of the added complexity (source code and runtime) of the feature.

I agree -Wsuggest-override should be off by default, yet I suspect its user-penetration will be much higher than other off-by-default warnings, due to numerous instances of people on the Internet asking for this feature, as well as the precedent for it set by GCC. Moreover, since this implementation of this warning lies along the exact same code paths as the already existing -Winconsistent-missing-override, the added complexity from this patch is absolutely minimal.

clang/lib/Sema/SemaDeclCXX.cpp
3075

Agreed. Good idea on the fix--needed one more line break (the first one still hit column 81), but it looks much better now.

clang/test/SemaCXX/warn-suggest-destructor-override
7

Because of void virtual? It does, surprisingly, as it does in the test for warn-inconsistent-missing-destructor-override, where I pilfered this from.

Nevertheless, changed to virtual void for sanity's sake.

Glad this is generating some discussion. For my $0.02, I would also (obviously) love to be able to enable this warning on all the codebases I work on, and this patch was born out of a discussion on the C++ Slack with another user who had found this warning very useful in GCC and was wondering why Clang didn't have it yet.

The issue is that such a warning then needs to be off by default, because we can't assume the user's intent. And Clang's historically been fairly averse to off-by-default warnings due to low user-penetration (not zero, like I said, I imagine LLVM itself would use such a warning, were it implemented) & so concerns about the cost/benefit tradeoff of the added complexity (source code and runtime) of the feature.

I agree -Wsuggest-override should be off by default, yet I suspect its user-penetration will be much higher than other off-by-default warnings, due to numerous instances of people on the Internet asking for this feature, as well as the precedent for it set by GCC.

Ah, I hadn't checked/wasn't mentioned whether GCC has this warning. If GCC already has it, there's usually an easy enough argument to be made for GCC compatibility being worthwhile that overcomes most of the objections unless the GCC warning is particularly problematic in some way that I doubt this is.

Is the implementation you're proposing fairly consistent with GCC's? Run it over any big codebases to check it warns in the same places GCC does?

Moreover, since this implementation of this warning lies along the exact same code paths as the already existing -Winconsistent-missing-override, the added complexity from this patch is absolutely minimal.

Is the implementation you're proposing fairly consistent with GCC's? Run it over any big codebases to check it warns in the same places GCC does?

This patch has the same behavior as -Wsuggest-override in GCC >= 9. In GCC <9, it would suggest adding override to void foo() final, but in GCC >=9, final is enough to suppress the warning. This patch's -Wsuggest-override, as well as Clang's pre-existing -Winconsistent-missing-override, are also silenced by final. (https://godbolt.org/z/hbxLK6)

I built Clang itself with a Clang that had this patch, and with GCC with -Wsuggest-override, and compared the results--they were identical (except for the warning text). (618 warnings, for those interested.)

logan-5 updated this revision to Diff 276162.Jul 7 2020, 11:41 AM

clang-formatted the diff.

Is the implementation you're proposing fairly consistent with GCC's? Run it over any big codebases to check it warns in the same places GCC does?

This patch has the same behavior as -Wsuggest-override in GCC >= 9. In GCC <9, it would suggest adding override to void foo() final, but in GCC >=9, final is enough to suppress the warning. This patch's -Wsuggest-override, as well as Clang's pre-existing -Winconsistent-missing-override, are also silenced by final. (https://godbolt.org/z/hbxLK6)

I built Clang itself with a Clang that had this patch, and with GCC with -Wsuggest-override, and compared the results--they were identical (except for the warning text). (618 warnings, for those interested.)

Awesome! Really appreciate the data.

I'm generally on board with this, but would like @rsmith 's sign off to be sure.

I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't feel /too/ strongly about it.

clang/lib/Sema/SemaDeclCXX.cpp
3064–3079

Generally I'd recommend default ref capture [&] on any lambda that doesn't escape its scope - normal scopes don't need to document which variables you use inside them, and I think the same applies to lambdas (bit more debatable when the lambda is named and called later, even within the same scope - so if you feel strongly about keeping it the way it is, that's OK)

clang/test/SemaCXX/warn-suggest-override
4–5

I'd probably simplify these tests by using struct, so everything's implicitly public, rather than class and having to make things public.

Also probably member function declarations rather than definitions would be simpler?

logan-5 marked 2 inline comments as done.Jul 7 2020, 12:27 PM

I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't feel /too/ strongly about it.

So, ironing this out would mean the code would need this structure:

if (Inconsistent && IsWarningEnabled(-Winconsistent-missing-override))
    Emit(-Winconsistent-missing-override);
else
    Emit(-Wsuggest-override);

The issue is that I wasn't able to find a way to ask if a warning is enabled and make a control flow decision based on that. If there is an API for doing this, it's hidden well. :) So I fell back to just doing if (Inconsistent) instead, which has this quirk if the inconsistent version of the warning is disabled.

clang/lib/Sema/SemaDeclCXX.cpp
3064–3079

I don't feel strongly at all; I'm fine with [&]. I'll make that change.

clang/test/SemaCXX/warn-suggest-override
4–5

Can do.

logan-5 updated this revision to Diff 276187.Jul 7 2020, 1:55 PM

Addressed minor feedback.

logan-5 marked 2 inline comments as done.Jul 7 2020, 1:55 PM

I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't feel /too/ strongly about it.

So, ironing this out would mean the code would need this structure:

if (Inconsistent && IsWarningEnabled(-Winconsistent-missing-override))
    Emit(-Winconsistent-missing-override);
else
    Emit(-Wsuggest-override);

The issue is that I wasn't able to find a way to ask if a warning is enabled and make a control flow decision based on that. If there is an API for doing this, it's hidden well. :) So I fell back to just doing if (Inconsistent) instead, which has this quirk if the inconsistent version of the warning is disabled.

Oh, yep, there's a way - it's usually used for performance-costly warnings, to not spend the resources computing the warning if it's disabled anyway.

Here's an arbitrary example: https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7237

Oh, yep, there's a way - it's usually used for performance-costly warnings, to not spend the resources computing the warning if it's disabled anyway.

Wow, thanks--overlooked that in a big way. That'll definitely solve the problem. Fixed up patch on the way.

logan-5 updated this revision to Diff 276248.Jul 7 2020, 3:47 PM

Fall back to -Wsuggest-override if -Winconsistent-missing-override is disabled.

Generally looks good to me (description/commit message should be updated now that the inconsistent inconsistency is no longer an issue)

clang/test/SemaCXX/warn-suggest-destructor-override
2

Does GCC have suggest-destructor-override as a separate warning too?

logan-5 edited the summary of this revision. (Show Details)Jul 7 2020, 5:35 PM
logan-5 marked an inline comment as done.Jul 7 2020, 5:41 PM
logan-5 added inline comments.
clang/test/SemaCXX/warn-suggest-destructor-override
2

It does not. In fact, there's no way to get GCC to warn about destructors missing override. (Which is somewhat defensible, since override is really great for preventing subtle signature mismatches/typos, but destructors don't really have those problems.) However, Clang already has a destructor-specific flavor of the inconsistent-override warning, so I added -Wsuggest-destructor-override for symmetry with -Winconsistent-missing-[destructor-]override.

logan-5 marked an inline comment as done.Jul 7 2020, 5:43 PM
logan-5 added inline comments.
clang/test/SemaCXX/warn-suggest-destructor-override
2

Note that this really is the best of both worlds, since it lets Clang's -Wsuggest-override behave identically to GCC's (not warning on destructors), as well as be consistent with its own already existing override warnings (with a special extra flag to enable warnings for destructors).

logan-5 edited the summary of this revision. (Show Details)Jul 9 2020, 10:00 AM

Feels like a dumb question, but I'm not sure if and how those build failures are related to this patch? They seem to involve completely separate parts of the LLVM project (and .c files to boot). Was it that the build was failing for an unrelated reason at the moment the bots happened to build this patch? Or am I misunderstanding something more obvious?

dblaikie accepted this revision.Sat, Jul 11, 4:42 PM

Feels like a dumb question, but I'm not sure if and how those build failures are related to this patch? They seem to involve completely separate parts of the LLVM project (and .c files to boot). Was it that the build was failing for an unrelated reason at the moment the bots happened to build this patch? Or am I misunderstanding something more obvious?

Yeah, don't think those are related, no. Wouldn't worry about them.

Looks good - thanks for the patch and all the details! Might be worth turning on by default in the LLVM build (after all the cleanup)

This revision is now accepted and ready to land.Sat, Jul 11, 4:42 PM

Looks good - thanks for the patch and all the details! Might be worth turning on by default in the LLVM build (after all the cleanup)

Thanks a lot! I don't (think I) have commit access yada yada, so I'd really appreciate any help getting this committed.

I've already got some of the cleanup in the works (D83611 and D83616), and was planning on taking care of the rest throughout the coming weekish. I'd be happy to turn this warning on in the LLVM build after that.

This revision was automatically updated to reflect the committed changes.

Is it possible to emit fixit note with "override" ?

Is it possible to emit fixit note with "override" ?

This is a good idea, though unfortunately (after eyeballing the implementation of modernize-use-override in clang-tidy (UseOverrideCheck.cpp)), it looks non-trivial to figure out where exactly to insert override. There's some significant logic in the clang-tidy check involving re-lexing the relevant tokens, to find the insertion point in the presence of complexity like inline definitions, = 0, = {delete|default}, function try blocks, macros, and the list goes on.

The clang-tidy check has this FIXME comment to address the complexity (UseOverrideCheck.cpp:136):

// FIXME: Instead of re-lexing and looking for specific macros such as
// 'ABSTRACT', properly store the location of 'virtual' and '= 0' in each
// FunctionDecl.

If this was done, the resulting information would presumably simplify the clang-tidy check as well as make it easier to add a fixit to this warning. This sounds like an interesting problem, but also like a sizable refactor, and I won't be upset if someone beats me to it. :)