<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Oct 30, 2014 at 9:21 AM, jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>></span> wrote:<br><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>
> On Oct 29, 2014, at 6:23 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
><br>
> 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>
><br>
> 1. This is a very cool warning.<br>
> 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>
><br>
> Thanks,<br>
> Nico<br>
><br>
<br>
<br>
</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>
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></blockquote><div><br></div><div>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> </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></blockquote><div><br></div><div>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><br></div><div><div>..\..\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> END_COM_MAP()</div><div> ^</div><div>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> virtual ULONG STDMETHODCALLTYPE AddRef(void) throw() = 0; \</div><div> ^</div><div>C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk/Include/um/unknwnbase.h(118,45) : note(clang): overridden virtual function is here</div><div> virtual ULONG STDMETHODCALLTYPE AddRef( void) = 0;</div><div> ^</div></div><div><br></div><div>The END_COM_MAP() call is in our code, but there's nothing we can do about it.</div><div> </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></blockquote></div><br></div><div class="gmail_extra">Sounds good :-)</div></div>