<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">In r<font face="Menlo" class=""><span style="font-size: 11px;" class="">220992. Hope this surpasses the warning as you expect.</span></font><div class=""><font face="Menlo" class=""><span style="font-size: 11px;" class="">- Fariborz</span></font></div><div class=""><font face="Menlo" class=""><span style="font-size: 11px;" class=""><br class=""></span></font><div style=""><blockquote type="cite" class=""><div class="">On Oct 30, 2014, at 3:06 PM, Nico Weber <<a href="mailto:thakis@chromium.org" class="">thakis@chromium.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">On Thu, Oct 30, 2014 at 9:21 AM, jahanian <span dir="ltr" class=""><<a href="mailto:fjahanian@apple.com" target="_blank" class="">fjahanian@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br class="">
> On Oct 29, 2014, at 6:23 PM, Nico Weber <<a href="mailto:thakis@chromium.org" class="">thakis@chromium.org</a>> wrote:<br class="">
><br class="">
> 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:<br class="">
><br class="">
> 1. This is a very cool warning.<br class="">
> 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?<br class="">
><br class="">
> Thanks,<br class="">
> Nico<br class="">
><br class="">
<br class="">
<br class="">
</span>To my (pleasant) surprise, there were not any builedbot issues of late. So, it seems that people are following the practice of adding ‘override’<br class="">
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?<br class=""></blockquote><div class=""><br class=""></div><div class="">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.)</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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.<br class=""></blockquote><div class=""><br class=""></div><div class="">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:</div><div class=""><br class=""></div><div class=""><div class="">..\..\win8/metro_driver/ime/text_store.h(102,3) : warning(clang): 'AddRef' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]</div><div class=""> END_COM_MAP()</div><div class=""> ^</div><div class="">C:\b\depot_tools\win_toolchain\vs2013_files/VC/atlmfc/include\atlcom.h(2358,34) : note(clang): expanded from macro 'END_COM_MAP'</div><div class=""> virtual ULONG STDMETHODCALLTYPE AddRef(void) throw() = 0; \</div><div class=""> ^</div><div class="">C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk/Include/um/unknwnbase.h(118,45) : note(clang): overridden virtual function is here</div><div class=""> virtual ULONG STDMETHODCALLTYPE AddRef( void) = 0;</div><div class=""> ^</div></div><div class=""><br class=""></div><div class="">The END_COM_MAP() call is in our code, but there's nothing we can do about it.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
But since Richard has reviewed the patch, I would like to hear from him as well.<br class=""></blockquote></div><br class=""></div><div class="gmail_extra">Sounds good :-)</div></div>
</div></blockquote></div><br class=""></div></body></html>