[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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.

In D82728#2134072 <https://reviews.llvm.org/D82728#2134072>, @dblaikie wrote:

> 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 <https://stackoverflow.com/a/41109550/5379590> this feature <https://stackoverflow.com/a/29154136/5379590>, 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.



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3075
+              : diag::
+                    warn_inconsistent_function_marked_not_override_overriding);
+    else
----------------
Quuxplusone wrote:
> 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);
> ```
Agreed. Good idea on the fix--needed one more line break (the first one still hit column 81), but it looks much better now.


================
Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:6
+  ~A() {}
+  void virtual run() {}
+};
----------------
Quuxplusone wrote:
> Surely this doesn't compile?!
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82728/new/

https://reviews.llvm.org/D82728