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

[Nico Weber]

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.


> But since Richard has reviewed the patch, I would like to hear from him as
> well.
>

Sounds good :-)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141030/1e1b3860/attachment.html>