<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="">LGTM<div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Sep 26, 2014, at 4:10 PM, jahanian <<a href="mailto:fjahanian@apple.com" class="">fjahanian@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div 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;" class=""><div class=""><br class="Apple-interchange-newline">On Sep 26, 2014, at 3:37 PM, Douglas Gregor <<a href="mailto:dgregor@apple.com" class="">dgregor@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br class=""><div class=""><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=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><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=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><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 class="" 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;"><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 class=""><div class="">+def warn_function_marked_not_override_overriding : Warning <</div><div class="">+ "%0 is not marked 'override' but overrides a member functions">,</div><div class="">+ 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></div></blockquote><div class=""><br class=""></div>Thanks for quick review. Here is the updated patch.</div><div 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;" class=""><span class="Apple-tab-span" style="white-space: pre;"> </span></div><span id="cid:FCC9EE81-7326-4684-BB1C-B2B4BC6656BF@apple.com"><override-patch.txt></span><div 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;" class=""></div><div 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;" class=""><br class=""></div><div 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;" class="">- Fariborz<br class=""><blockquote type="cite" class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class=""><div class=""><br class=""></div></div><div class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>- Doug</div></div></blockquote></div></div></blockquote></div><br class=""></div></body></html>