<div dir="ltr"><div><div>+cc clang-tidy people, since they rolled this kind of stuff out already.</div><div><br></div><div>Index: test/Parser/cxx0x-in-cxx98.cpp</div><div>===================================================================</div><div>--- test/Parser/cxx0x-in-cxx98.cpp<span class="" style="white-space:pre"> </span>(revision 218519)</div><div>+++ test/Parser/cxx0x-in-cxx98.cpp<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -10,11 +10,12 @@</div><div> </div><div> struct B {</div><div> virtual void f();</div><div>- virtual void g();</div><div>+ virtual void g(); // expected-note {{overridden virtual function is here}}</div><div> };</div><div> struct D final : B { // expected-warning {{'final' keyword is a C++11 extension}}</div><div> virtual void f() override; // expected-warning {{'override' keyword is a C++11 extension}}</div><div>- virtual void g() final; // expected-warning {{'final' keyword is a C++11 extension}}</div><div>+ virtual void g() final; // expected-warning {{'final' keyword is a C++11 extension}} \</div><div>+<span class="" style="white-space:pre"> </span> // expected-warning {{'g' overrides a member function but is not marked 'override'}}</div><div> };</div><div> </div><div> void NewBracedInitList() {</div></div><div><br></div>Should we really be suggesting that users add C++11 features like override when -Wcxx98-compat is on? That seems undesirable.<div><br></div><div><div>Index: test/Parser/cxx0x-decl.cpp</div><div>===================================================================</div><div>--- test/Parser/cxx0x-decl.cpp<span class="" style="white-space:pre"> </span>(revision 218519)</div><div>+++ test/Parser/cxx0x-decl.cpp<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -83,13 +83,13 @@</div><div> </div><div> namespace FinalOverride {</div><div> struct Base {</div><div>- virtual void *f();</div><div>+ virtual void *f(); // expected-note {{overridden virtual function is here}}</div><div> virtual void *g();</div><div> virtual void *h();</div><div> virtual void *i();</div><div> };</div><div> struct Derived : Base {</div><div>- virtual auto f() -> void *final;</div><div>+ virtual auto f() -> void *final; // expected-warning {{'f' overrides a member function but is not marked 'override'}}</div><div> virtual auto g() -> void *override;</div><div> virtual auto h() -> void *final override;</div><div> virtual auto i() -> void *override final;</div></div><div><br></div><div>Why make this suggestion? 'override' is redundant in the presence of 'final', assuming that we warn on final virtual methods that don't override anything, which we probably should.</div><div><br></div><div><div>Index: test/SemaCXX/ms-interface.cpp</div><div>===================================================================</div><div>--- test/SemaCXX/ms-interface.cpp<span class="" style="white-space:pre"> </span>(revision 218519)</div><div>+++ test/SemaCXX/ms-interface.cpp<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -12,7 +12,7 @@</div><div> operator int();</div><div> // expected-error@+1 {{nested class I1::(anonymous) is not permitted within an interface type}}</div><div> struct { int a; };</div><div>- void fn2() {</div><div>+ void fn2() { // expected-note {{overridden virtual function is here}}</div><div> struct A { }; // should be ignored: not a nested class</div><div> }</div><div> protected: // expected-error {{interface types cannot specify 'protected' access}}</div><div>@@ -44,7 +44,7 @@</div><div> __interface I4 : I1, I2 {</div><div> void fn1() const override;</div><div> // expected-error@+1 {{'final' keyword not permitted with interface types}}</div><div>- void fn2() final;</div><div>+ void fn2() final; // expected-warning {{'fn2' overrides a member function but is not marked 'override'}}</div><div> };</div><div> </div><div> // expected-error@+1 {{interface type cannot inherit from non-public 'interface I1'}}</div></div><div><br></div><div>ditto</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 30, 2014 at 2:45 PM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>+def CXX11WarnOverrideMethod : DiagGroup<"warn-missing-override-on-overriding-method">;</div><div><br></div><div>Anyone care to bikeshed the name a bit? This flag feels really verbose, especially given that it gets a -W prefix.</div><div><br></div><div>Can we go with -Wmissing-override or -Winconsistent-missing-override to reflect the fact that it's heuristically triggered based on use of C++11 override somewhere in the class hierarchy?</div></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Fri, Sep 26, 2014 at 4:15 PM, jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><div style="word-wrap:break-word"><br><div><div>On Sep 26, 2014, at 4:12 PM, Douglas Gregor <<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word">LGTM<div><br></div></div></blockquote>Thanks. I want to add FixIts before checking it in.</div><div><br></div><div>- Fariborz</div><div><div><div><br><blockquote type="cite"><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div>On Sep 26, 2014, at 4:10 PM, jahanian <<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>> wrote:</div><br><div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div><br>On Sep 26, 2014, at 3:37 PM, Douglas Gregor <<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Sep 26, 2014, at 3:03 PM, jahanian <<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>> wrote:</div><br><div><div style="word-wrap:break-word"><br><div><div>On Sep 25, 2014, at 11:51 AM, Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Sep 25, 2014, at 11:24 AM, Douglas Gregor <<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>> wrote:</div><br><div><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important"><span> </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><div><div>This seems like a great idea to me!</div><div>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><br></div></div></blockquote></div><br><div>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></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><br></div><div>“a member functions” shouldn’t be plural. Also, perhaps we should turn this around:</div><div><br></div><div><span style="white-space:pre-wrap"> </span>“%0 overrides a member function but is not marked ‘override’”</div><div><br></div><div>because that puts the context of the problem before the problem.</div><div><br></div><div><div>+ if (HasMethodWithOverrideControl) {</div><div>+ // At list one method has the 'override' control declared.</div><div>+ // Diagnose all other overridden methods which do not have 'override' specified on them.</div></div><div><div>+ for (auto *M : Record->methods())</div></div><div><br></div><div>“At list” -> “At least”.</div><div><br></div><div>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><br></div><div><div>+ if (M->hasAttr<OverrideAttr>())</div><div>+ HasMethodWithOverrideControl = true;</div></div><div><br></div><div>with</div><div><br></div><div><span style="white-space:pre-wrap"> </span>else if (M->begin_overridden_methods() != M->end_overridden_methods())</div><div><span style="white-space:pre-wrap"> </span> HasOverridingMethodWithoutOverrideControl = true;</div><div><br></div><div>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><br></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;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="white-space:pre-wrap"> </span></div><span><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;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">- Fariborz<br><blockquote type="cite"><div style="word-wrap:break-word"><div><div><br></div></div><div><span style="white-space:pre-wrap"> </span>- Doug</div></div></blockquote></div></div></blockquote></div><br></div></div></blockquote></div><br></div></div></div><br></div></div><span class="">_______________________________________________<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></span></blockquote></div><br></div>
</blockquote></div><br></div>