r218925 - Patch to warn if 'override' is missing

[Richard Smith]

On 3 Oct 2014 10:13, "jahanian" <fjahanian at apple.com> wrote:
>
>
> On Oct 2, 2014, at 5:24 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
>> On Thu, Oct 2, 2014 at 4:13 PM, Fariborz Jahanian <fjahanian at apple.com>
wrote:
>>>
>>> Author: fjahanian
>>> Date: Thu Oct  2 18:13:51 2014
>>> New Revision: 218925
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=218925&view=rev
>>> Log:
>>> Patch to warn if 'override' is missing
>>> for an overriding method if class has at least one
>>> 'override' specified on one of its methods.
>>> Reviewed by Doug Gregor. rdar://18295240
>>> (I have already checked in all llvm files with missing 'override'
>>>  methods and Bob Wilson has fixed a TableGen of FastISel so
>>>  no warnings are expected from build of llvm after this patch.
>>>  I have already verified this).
>>>
>>> Added:
>>>     cfe/trunk/test/FixIt/cxx11-fixit-missing-override.cpp
>>>     cfe/trunk/test/SemaCXX/cxx11-warn-missing-override.cpp
>>> Modified:
>>>     cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>>>     cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>>     cfe/trunk/include/clang/Sema/Sema.h
>>>     cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>>>     cfe/trunk/test/CXX/class.derived/class.virtual/p3-0x.cpp
>>>     cfe/trunk/test/FixIt/fixit-cxx0x.cpp
>>>     cfe/trunk/test/Parser/MicrosoftExtensions.cpp
>>>     cfe/trunk/test/Parser/cxx0x-decl.cpp
>>>     cfe/trunk/test/Parser/cxx0x-in-cxx98.cpp
>>>     cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp
>>>     cfe/trunk/test/SemaCXX/attr-gnu.cpp
>>>     cfe/trunk/test/SemaCXX/cxx98-compat.cpp
>>>     cfe/trunk/test/SemaCXX/ms-interface.cpp
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>>> URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=218925&r1=218924&r2=218925&view=diff
>>>
==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Oct  2
18:13:51 2014
>>> @@ -146,6 +146,8 @@ def CXX98CompatPedantic : DiagGroup<"c++
>>>
>>>  def CXX11Narrowing : DiagGroup<"c++11-narrowing">;
>>>
>>> +def CXX11WarnOverrideMethod :
DiagGroup<"inconsistent-missing-override">;
>>> +
>>>  // Original name of this warning in Clang
>>>  def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
>>>
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=218925&r1=218924&r2=218925&view=diff
>>>
==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Oct  2
18:13:51 2014
>>> @@ -1689,6 +1689,9 @@ def override_keyword_hides_virtual_membe
>>>    "%select{function|functions}1">;
>>>  def err_function_marked_override_not_overriding : Error<
>>>    "%0 marked 'override' but does not override any member functions">;
>>> +def warn_function_marked_not_override_overriding : Warning <
>>> +  "%0 overrides a member function but is not marked 'override'">,
>>> +  InGroup<CXX11WarnOverrideMethod>;
>>>  def err_class_marked_final_used_as_base : Error<
>>>    "base %0 is marked '%select{final|sealed}1'">;
>>>  def warn_abstract_final_class : Warning<
>>>
>>> Modified: cfe/trunk/include/clang/Sema/Sema.h
>>> URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=218925&r1=218924&r2=218925&view=diff
>>>
==============================================================================
>>> --- cfe/trunk/include/clang/Sema/Sema.h (original)
>>> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Oct  2 18:13:51 2014
>>> @@ -5084,6 +5084,10 @@ public:
>>>
>>>    /// CheckOverrideControl - Check C++11 override control semantics.
>>>    void CheckOverrideControl(NamedDecl *D);
>>> +
>>> +  /// DiagnoseAbsenseOfOverrideControl - Diagnose if override control
was
>>> +  /// not used in the; otherwise, overriding method.
>>
>>
>> Something weird has happened to this comment.
>>
>>>
>>> +  void DiagnoseAbsenseOfOverrideControl(NamedDecl *D);
>>>
>>>    /// CheckForFunctionMarkedFinal - Checks whether a virtual member
function
>>>    /// overrides a virtual member function marked 'final', according to
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>>> URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=218925&r1=218924&r2=218925&view=diff
>>>
==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Thu Oct  2 18:13:51 2014
>>> @@ -1897,6 +1897,31 @@ void Sema::CheckOverrideControl(NamedDec
>>>        << MD->getDeclName();
>>>  }
>>>
>>> +void Sema::DiagnoseAbsenseOfOverrideControl(NamedDecl *D) {
>>> +  if (D->isInvalidDecl())
>>> +    return;
>>> +  CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D);
>>> +  if (!MD || MD->isImplicit() || isa<CXXDestructorDecl>(MD))
>>> +    return;
>>
>>
>> Why do you skip destructors?
>
>
> Because I found that allowing it makes the warning much noisier and for
destructors (with not having parameters)
> issuing the warning for them does not make it all that useful.
>>
>>
>>> +  bool HasOverriddenMethods =
>>> +    MD->begin_overridden_methods() != MD->end_overridden_methods();
>>> +  if (HasOverriddenMethods) {
>>> +    SourceLocation EndLocation =
>>> +      (MD->isPure() || MD->hasAttr<FinalAttr>())
>>
>>
>> We should not warn if the function (or the class) has the 'final'
attribute.
>
> This came up before and Doug provided this response:
>
> "That’s not true. “override” says that you’re overriding something from
the base class, potentially changing the behavior from what your base class
would have provided. “final” says that your own derived classes can’t
change the behavior further. Those are separate concerns.”

I respectfully disagree. In the absence of virtual, final implies override.
(And mixing final with virtual is a bad idea they we should probably warn
on. And mixing final with override is rightly banned by several style
guides already.)

> But a bigger question is if there is objection to having this kind of
potentially noisy warning? There will be projects which break and patch
gets reverted.

I think it's fine if it's done right. I don't think the current approach is
viable.

>
> - Fariborz
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141003/002c9622/attachment.html>