On Thu, Oct 30, 2014 at 3:06 PM, Nico Weber <tha...@chromium.org> wrote:
> On Thu, Oct 30, 2014 at 9:21 AM, jahanian <fjahan...@apple.com> wrote: > >> >> > On Oct 29, 2014, at 6:23 PM, Nico Weber <tha...@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.
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
