In r220992. Hope this surpasses the warning as you expect. - Fariborz > On Oct 30, 2014, at 3:06 PM, Nico Weber <[email protected]> wrote: > > On Thu, Oct 30, 2014 at 9:21 AM, jahanian <[email protected] > <mailto:[email protected]>> wrote: > > > On Oct 29, 2014, at 6:23 PM, Nico Weber <[email protected] > > <mailto:[email protected]>> 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 :-)
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
