<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Sep 26, 2014, at 3:03 PM, jahanian <<a href="mailto:fjahanian@apple.com" class="">fjahanian@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=windows-1252" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><div class="">On Sep 25, 2014, at 11:51 AM, Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" class="">kyrtzidis@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Sep 25, 2014, at 11:24 AM, Douglas Gregor <<a href="mailto:dgregor@apple.com" class="">dgregor@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""><span class="Apple-converted-space"> </span>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. 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.</span></div></blockquote></div><br class=""><div class=""><div class="">This seems like a great idea to me!</div><div class="">For the 'override' I much prefer if it is class specific to make it less of a burden as an “always on” warning. We could have the checking done at the end of the class definition.</div></div><div class=""><br class=""></div></div></blockquote></div><br class=""><div class="">Here is the patch. Warning is on by default. Number of new warnings on clang tests is greatly reduced but there are still some.</div></div></div></blockquote><br class=""></div><div><div>+def warn_function_marked_not_override_overriding : Warning <</div><div>+ "%0 is not marked 'override' but overrides a member functions">,</div><div>+ InGroup<CXX11WarnOverrideMethod>;</div><div class=""><br class=""></div><div class="">“a member functions” shouldn’t be plural. Also, perhaps we should turn this around:</div><div class=""><br class=""></div><div class=""><span class="Apple-tab-span" style="white-space:pre"> </span>“%0 overrides a member function but is not marked ‘override’”</div><div class=""><br class=""></div><div class="">because that puts the context of the problem before the problem.</div><div class=""><br class=""></div><div class=""><div class="">+ if (HasMethodWithOverrideControl) {</div><div class="">+ // At list one method has the 'override' control declared.</div><div class="">+ // Diagnose all other overridden methods which do not have 'override' specified on them.</div></div><div class=""><div class="">+ for (auto *M : Record->methods())</div></div><div class=""><br class=""></div><div class="">“At list” -> “At least”.</div><div class=""><br class=""></div><div class="">Also, this means we’ll be taking two passes over the methods if any “override” is present, even though we won’t often warn here. How about extending this:</div><div class=""><br class=""></div><div class=""><div class="">+ if (M->hasAttr<OverrideAttr>())</div><div class="">+ HasMethodWithOverrideControl = true;</div></div><div class=""><br class=""></div><div class="">with</div><div class=""><br class=""></div><div class=""><span class="Apple-tab-span" style="white-space:pre"> </span>else if (M->begin_overridden_methods() != M->end_overridden_methods())</div><div class=""><span class="Apple-tab-span" style="white-space:pre"> </span> HasOverridingMethodWithoutOverrideControl = true;</div><div class=""><br class=""></div><div class="">and we only do this second pass when we know we’re going to warn, e.g., if HasMethodWithOverrideControl && HasOverridingMethodWithoutOverrideControl?</div><div class=""><br class=""></div></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><br class=""></div><br class=""></body></html>