<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 25, 2014 at 11:24 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Sep 24, 2014, at 4:54 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 24, 2014 at 4:42 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>This isn’t a style checker, this is to make sure you are not overriding something by accident.</div></div></blockquote><div><br></div><div>For context, I took those quotations from a thread about a warning for "using a null pointer other than nullptr" which seems pretty analogous to this warning. "Use this feature because it might help you find bugs" but I think Doug was basically saying that the guideline to use a particular feature is a stylistic one, even if that style then leads to finding bugs.</div></div></div></div></div></blockquote><div><br></div></span><div>Yeah, both warnings fall into the “introduce extra syntax, the consistent use of which will prevent bugs” camp. One can choose to use the feature if the syntactic overhead is less burdensome than tracking down the bugs, or not. </div><div><br></div><div>My position has softened somewhat, and I don’t feel as strongly that these warnings is “purely stylistic”. I still think an off-by-default warning is extremely unfortunate, because new warning flags aren’t that discoverable. I’d feel a lot better if some part of the warning could be on by default. For example, if you’ve uttered “override” at least once in a class, it makes sense to warn-by-default about any other overrides in that class that weren’t marked as “override”, because you’re being locally inconsistent. </div></div></div></blockquote><div><br></div><div>I believe James Dennett had the same idea - filed a bug internally which we never got around to filing externally/fixing. I tend to agree that that seems like a totally reasonable indicator/signal and "overriding functions without explicit override in a class with at least one explicit override" are probably mistakes, if not outright bugs, of some degree that could reasonably classified as a "true positive".</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>Or maybe you can expand that heuristic out to a file-level granularity (which matches better for the null point constant warning) and still be on-by-default.</div></div></div></blockquote><div><br></div><div>Same file would be interesting when it's not just the primary source file but could just be the same header, etc, sounds reasonable to me. (sort of going to hate to be the guy that adds the first nullptr to a source file & have the build light up like a christmas tree as every NULL in the file produces a warning/error, though... )<br><br>That might be tricky to introduce another delayed diagnostic machine which has to wait & see if there's an override/nullptr anywhere in the file, then go back & emit the delayed diagnostics for this purpose.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div> Explicitly providing the warning flag (or some other related warning flag) could turn the warning on everywhere, for those who want the rule consistently applied to their code base.</div><div><br></div><span style="white-space:pre-wrap"> </span>- Doug</div><div><div class="h5"><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>It needs to be off-by-default due to the vast amount of C++ code that exists out there, but we can have it enabled when you create a new project.</div><div><div><br><div><blockquote type="cite"><div>On Sep 24, 2014, at 4:16 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 24, 2014 at 3:57 PM, 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">Hi all,<br>
<br>
We have an enhancement request  from our users to provide<br>
warning if ‘override’ control is missing. This warning is off by default as it will<br>
be noisy (but it will show itself with -Weverything).<br>
Is such a patch useful enough to go into the trunk? Also, comment on the patch is appreciated.<br>
I will provide ‘fixit’ later if this is a worthwhile patch.<br></blockquote><div><br></div><div>While I rather like the idea of such a warning, the usual bar has been a strong aversion to adding off-by-default warnings. I think the theoretical future might be building warnings like this into clang-tidy, then providing some plugin-like option to enable certain clang-tidy warnings in your normal builds.<br><br>(because I was curious, I went back & found some choice quotes from Doug Gregor on warnings like this (this is what he told me, years ago, when I proposed adding a warning for null pointers that aren't nullptr*):<br><br><div><div><font face="arial, helvetica, sans-serif"><span>"Off</span>-by-<span>default</span> <span>warnings</span> are not a mechanism to subvert our normal processes for vetting a<span>warning</span>. In general, we should avoid <span>off</span>-by-<span>default</span> <span>warnings</span>: if it's not good enough to turn on by <span>default</span>, why do we have it at all? The vast majority of users will never see an <span>off</span>-by-<span>default</span> <span>warning</span>."</font></div><div><font face="arial, helvetica, sans-serif">"A compiler is not a style checker, nor should it ever be."</font></div></div><div><font face="arial, helvetica, sans-serif"><span>"Warnings</span> are intended to find potential problems in the source code. Style migration is the domain of separate tools.")<br><br>(& cc'ing Doug in case there's something about this that's different/things have changed over the years)</font></div><br>* today, I'd probably be able to get that in on the basis of compatibility with GCC's <span style="font-family:monospace;font-size:inherit">-Wzero-as-null-pointer-constant</span></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">
<br>
- Fariborz<br>
<br>
        <br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></div></blockquote></div><br></div></div></div></blockquote></div><br></div></div>
</div></blockquote></div><br></div></div></div></blockquote></div><br></div></div>