r220703 - c++11 patch to issue warning on missing 'override' on

[Richard Smith]

On Thu, Oct 30, 2014 at 3:06 PM, Nico Weber <thakis at chromium.org> wrote:

> On Thu, Oct 30, 2014 at 9:21 AM, jahanian <fjahanian at apple.com> wrote:
>
>>
>> > On Oct 29, 2014, at 6:23 PM, Nico Weber <thakis at chromium.org> wrote:
>> >
>> > I've now looked at a bit over 1000 instances of this warning (and fixed
>> many of them); with this experience I have two more pieces of feedback:
>> >
>> > 1. This is a very cool warning.
>> > 2. In practice, the methods this warns on sometimes comes from a macro
>> and it's impossible to add override. Examples are gmock's MOCK_METHODn, and
>> IFACEMETHOD and COM_INTERFACE_ENTRY from the Windows SDK. Maybe this
>> shouldn't warn if the method it warns on comes from a macro expansion? Or,
>> if you think that that would hide too many issues (it did find a few cases
>> where I could just add "override" to a macro, in my experience), maybe at
>> least if the macro that's expanded is from a system header?
>> >
>> > Thanks,
>> > Nico
>> >
>>
>>
>> To my (pleasant) surprise, there were not any builedbot issues of late.
>> So, it seems that people are following the practice of adding ‘override’
>> before this patch hit (of course I made sure that llvm builds do not warn
>> beforehand). Would it be hard to fix Windows SDK’s macros to add this
>> keyword?
>>
>
> Probably not for people at Microsoft, but for people just using the SDK
> (like me) it's nigh impossible. (And even if the SDK got fixed, projects
> won't be able to update to the newest SDK immediately.)
>
>
>> One practice has been to not warn if condition occurs in system headers
>> (as you suggested). This is my preference should we decide to restrict this
>> warning.
>>
>
> The diag system automatically suppresses warnings in system headers. What
> I mean is if the warning is reported on a macro expansion on user code, but
> the macro is from a system header. Here's a concrete example:
>
> ..\..\win8/metro_driver/ime/text_store.h(102,3) :  warning(clang):
> 'AddRef' overrides a member function but is not marked
> 'override' [-Winconsistent-missing-override]
>   END_COM_MAP()
>   ^
> C:\b\depot_tools\win_toolchain\vs2013_files/VC/atlmfc/include\atlcom.h(2358,34)
> :  note(clang): expanded from macro 'END_COM_MAP'
>         virtual ULONG STDMETHODCALLTYPE AddRef(void) throw() = 0; \
>                                         ^
> C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk/Include/um/unknwnbase.h(118,45)
> :  note(clang): overridden virtual function is here
>             virtual ULONG STDMETHODCALLTYPE AddRef( void) = 0;
>                                             ^
>
> The END_COM_MAP() call is in our code, but there's nothing we can do about
> it.
>

It seems sensible to me for us to suppress the warning in this case.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141030/cf769f45/attachment.html>