<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></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 Oct 24, 2014, at 12:04 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Sorry for the delay. LGTM subject to a few comments:</div></div></blockquote><div><br class=""></div>Thanks for the review. In r<span style="font-family: Menlo; font-size: 11px;" class="">220703.</span><div><span style="font-family: Menlo; font-size: 11px;" class="">- Fariborz</span></div><div><span style="font-family: Menlo; font-size: 11px;" class=""><br class=""></span></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class=""><div class="gmail_extra"><span style="font-family: 'Courier New', Courier, monospace; font-size: 14px; white-space: pre-wrap;" class="">+ if (HasMethodWithOverrideControl
+ && HasOverridingMethodWithoutOverrideControl) {
</span></div><div class=""><span style="font-family: 'Courier New', Courier, monospace; font-size: 14px; white-space: pre-wrap;" class=""><br class=""></span></div><div class="gmail_extra">The && should be on the previous line.</div><div class="gmail_extra"><br class=""></div><div class="gmail_extra"><span style="font-family: 'Courier New', Courier, monospace; font-size: 14px; white-space: pre-wrap;" class="">+ if (!M->hasAttr<OverrideAttr>())
+ DiagnoseAbsenceOfOverrideControl(M);
</span></div><div class=""><span style="font-family: 'Courier New', Courier, monospace; font-size: 14px; white-space: pre-wrap;" class=""><br class=""></span></div><div class="gmail_extra">It seems strange to me to put the check for OverrideAttr here but put the check for FinalAttr and overridden functions and so on in DiagnoseAbsenceOfOverrideControl. Move this check into the function with the others?</div><div class="gmail_extra"><br class=""></div><div class="gmail_extra"><span style="font-family: 'Courier New', Courier, monospace; font-size: 14px; white-space: pre-wrap;" class=""> // expected-error@+1 {{declaration of 'SealedFunction' overrides a 'sealed' function}}
- virtual void SealedFunction();
+ virtual void SealedFunction(); // expected-warning {{'SealedFunction' overrides a member function but is not marked 'override'}}
</span></div><div class=""><span style="font-family: 'Courier New', Courier, monospace; font-size: 14px; white-space: pre-wrap;" class=""><br class=""></span></div><div class="gmail_extra">It would be nice to suppress the warning if we're also issuing an error for overriding a 'final' function. Please at least add a FIXME to the test for that so that someone fixing this later doesn't think this is deliberate.</div><div class="gmail_extra"><br class=""></div></div></div></div></blockquote></div><div><span style="font-family: Menlo; font-size: 11px;" class=""><br class=""></span></div><br class=""></body></html>