Closed Bug 1158776 Opened 9 years ago Closed 6 years ago

Fix the usage of override and final keywords in the tree

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ehsan.akhgari, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

This patch has been automatically generated using clang-tidy's
misc-use-override check.  It adds override keywords where they were
missing, and removes them if the final keyword is also specified, in
which case override is redundant.
Attachment #8597955 - Flags: review?(dholbert)
Assignee: nobody → ehsan
Comment on attachment 8597955 [details] [diff] [review]
Fix the usage of override and final keywords in the tree

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #0)
> It adds override keywords where they were
> missing, and removes them if the final keyword is also specified, in
> which case override is redundant.

Two problems with this, the first stylistic & the second more important:

(1) This s/final override/final fixup seems to leave behind a trailing blank space (before the ";"), e.g. here:

>+++ b/dom/base/Element.h
>-  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) final override;
>+  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) final ;
>+++ b/dom/html/HTMLMediaElement.h
>   virtual void MetadataLoaded(const MediaInfo* aInfo,
>-                              nsAutoPtr<const MetadataTags> aTags) final override;
>+                              nsAutoPtr<const MetadataTags> aTags) final ;

(2) Override is *not* redundant in these cases, as you say it is.  The "final" keyword only ensures that the function is virtual -- it doesn't verify that it overrides anything. Put another way: "virtual DoStuff() final;" (and the final function-signatures above) are making zero assertions about overriding. If the inherited function changes or goes away, these functions will continue to compile just fine, and that's bad.

I'll post a c++ testcase to demonstrate more clearly.

So, r- on the patch for now, due to (2).
Attachment #8597955 - Flags: review?(dholbert) → review-
For reference, we discussed this same issue RE override & final previously in bug 1125673 comment 2 & following. tl;dr: I think it'd *only* make sense to treat 'final' & 'override' as mutually-exclusive/redundant if we consistently omitted the 'virtual' keyword from overriding function signatures. (The quoted lines in comment 2 show that we do not omit the 'virtual' keyword; hence, final & override are not redundant.)
First things first, clang-tidy's misc-use-override checker only looks at functions that are actually overridden, so in these cases the override keyword is actually redundant.

FWIW, right now clang-tidy drops the virtual keyword where the function is marked as override and/or final, so what it does in that case is correct.  I implemented an option <http://reviews.llvm.org/D9285> which tells clang-tidy to leave the virtual keyword alone, but it still only looks at overridden functions, so I think this transformation is correct.

But the more important question is, should we relax our coding style requirement to ask dropping the virtual keyword for overrides?

I think that if we keep our current coding style, dropping the override keyword when the virtual keyword is not present is dangerous since if someone comes around later and adds a virtual keyword they have changed the meaning of the program, which violates the assumptions of most C++ programmers.  Also, our coding style makes it difficult to identify whether |virtual void foo() final;| is overridden or not, which is an issue for readability (and in fact, even though I think the transformations are correct, they are probably not ideal because of this issue, so I am inclined to address it somehow.

So, I see two options here:

1. Relaxing our coding style to disallow the usage of virtual and override keywords together.
2. Extend my patched up clang-tidy to teach it to aggressively add virtual keywords for overrides unconditionally, and to never drop override when final is present.

What do you prefer?  Please feel free to suggest alternatives if you can think of them as well!

(Note that the good thing is that since we now have this tool in our hand, no matter what option we choose we can enforce it across the tree automatically!)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> FWIW, right now clang-tidy drops the virtual keyword where the function is
> marked as override and/or final, so what it does in that case is correct.  I
> implemented an option <http://reviews.llvm.org/D9285> which tells clang-tidy
> to leave the virtual keyword alone

Ah -- and I take it you used this option when generating this patch. (which is why 'virtual' stuck around in comment 2) (Makes sense.)

> but it still only looks at overridden
> functions, so I think this transformation is correct.

"Correct" is a bit of a relative term here. I think you're using it to mean "this transformation won't convert not-compiling code into compiling code"?

That may be true, but that doesn't mean the transformation is desirable -- it prevents the compiler from helping us in certain ways.  It means that later changes (e.g. changing the method signature in the superclass without modifying the subclass) would no longer be caught by the compiler as an error. And that's bad, because that's largely the point of using the 'override' keyword.  So from that perspective, the transformation is not correct. (or not desirable, at least)

> I think that if we keep our current coding style, dropping the override
> keyword when the virtual keyword is not present is dangerous since if
> someone comes around later and adds a virtual keyword they have changed the
> meaning

Agreed.

> Also, our coding style makes it difficult to identify whether
> |virtual void foo() final;| is overridden or not, which is an issue for
> readability

Agreed.

> So, I see two options here:
> 
> 1. Relaxing our coding style to disallow the usage of virtual and override
> keywords together.
> 2. Extend my patched up clang-tidy to teach it to aggressively add virtual
> keywords for overrides unconditionally, and to never drop override when
> final is present.
> 
> What do you prefer?

I prefer #2, as an incremental step forward at least.

(It seems like #1 would represent a much more substantial change to our coding style, particularly given that macros like NS_IMETHOD make us use the 'virtual' annotation in overriding method declarations all over the place. I suppose it's easy enough to change that macro-definition, though; and this would mean that NS_IMETHOD & NS_IMETHODIMP would end up being the same, which would mean we could remove NS_IMETHODIMP entirely I think -- and that'd be kind of nice.  It'd also be convenient that we'd end up matching Google's coding style on this point*, so people writing browser patches would have fewer conflicting styles to keep track of.  You'd want to get feedback from bsmedberg & dev.platform before going down this route, of course, though.
* https://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=The__define_Guard#Inheritance
)
(In reply to Daniel Holbert [:dholbert] from comment #6)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #5)
> > FWIW, right now clang-tidy drops the virtual keyword where the function is
> > marked as override and/or final, so what it does in that case is correct.  I
> > implemented an option <http://reviews.llvm.org/D9285> which tells clang-tidy
> > to leave the virtual keyword alone
> 
> Ah -- and I take it you used this option when generating this patch. (which
> is why 'virtual' stuck around in comment 2) (Makes sense.)

Yes, indeed.

> > but it still only looks at overridden
> > functions, so I think this transformation is correct.
> 
> "Correct" is a bit of a relative term here. I think you're using it to mean
> "this transformation won't convert not-compiling code into compiling code"?
> 
> That may be true, but that doesn't mean the transformation is desirable --
> it prevents the compiler from helping us in certain ways.  It means that
> later changes (e.g. changing the method signature in the superclass without
> modifying the subclass) would no longer be caught by the compiler as an
> error. And that's bad, because that's largely the point of using the
> 'override' keyword.  So from that perspective, the transformation is not
> correct. (or not desirable, at least)

Oh, that's a good point.  Again, this is related to keeping the virtual keyword, since for example if you have this setup:

  struct B {
    virtual void f();
  };
  struct D : B {
    void f() final;
  };

Then for example renaming B::f to B::g will cause D::f not compile with "error: only virtual member functions can be marked 'final'".  But if D::f uses the virtual keyword, then changing B::f will not be caught by the compiler.

> > So, I see two options here:
> > 
> > 1. Relaxing our coding style to disallow the usage of virtual and override
> > keywords together.
> > 2. Extend my patched up clang-tidy to teach it to aggressively add virtual
> > keywords for overrides unconditionally, and to never drop override when
> > final is present.
> > 
> > What do you prefer?
> 
> I prefer #2, as an incremental step forward at least.
> 
> (It seems like #1 would represent a much more substantial change to our
> coding style, particularly given that macros like NS_IMETHOD make us use the
> 'virtual' annotation in overriding method declarations all over the place. I
> suppose it's easy enough to change that macro-definition, though; and this
> would mean that NS_IMETHOD & NS_IMETHODIMP would end up being the same,
> which would mean we could remove NS_IMETHODIMP entirely I think -- and
> that'd be kind of nice.  It'd also be convenient that we'd end up matching
> Google's coding style on this point*, so people writing browser patches
> would have fewer conflicting styles to keep track of.  You'd want to get
> feedback from bsmedberg & dev.platform before going down this route, of
> course, though.
> *
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.
> html?showone=The__define_Guard#Inheritance
> )

OK.  I think given that #2 will require some more work on my part, I want to get feedback from folks on changing our coding style now, to see if I can avoid doing that work as just a temporary measure.  I'll post to dev-platform.
Sounds good to me.

(I was originally leaning against #1 because it'd require more substantial code changes, & because it'd would require more discussion/input before proceeding.  But in retrospect, I think #1's code-changes would all be automatable; whereas #2 requires new code to be written by a human. So, I'm on board with #1 for now, pending stylistic feedback.)
(If you do get buy-in on strategy #1 -- dropping 'virtual' where it's implied -- we should add some static analysis to ensure that at most one of 'virtual', 'override', & 'final' are ever used, to prevent the "someone comes around later and adds a virtual keyword" scenario in comment 5.)
(In reply to Daniel Holbert [:dholbert] from comment #8)
> Sounds good to me.
> 
> (I was originally leaning against #1 because it'd require more substantial
> code changes, & because it'd would require more discussion/input before
> proceeding.  But in retrospect, I think #1's code-changes would all be
> automatable; whereas #2 requires new code to be written by a human. So, I'm
> on board with #1 for now, pending stylistic feedback.)

Yeah indeed, #1 is technically easier since the kind robots will do all of the work.  :-)

(In reply to Daniel Holbert [:dholbert] from comment #9)
> (If you do get buy-in on strategy #1 -- dropping 'virtual' where it's
> implied -- we should add some static analysis to ensure that at most one of
> 'virtual', 'override', & 'final' are ever used, to prevent the "someone
> comes around later and adds a virtual keyword" scenario in comment 5.)

Did you mean "at least"?

If we end up maintaining the current style, I'll change clang-tidy to aggressively add the virtual keyword, so I don't think that static analysis would be needed (assuming that we run clang-tidy regularly on the code base, which is a requirement that I need to figure out anyway.)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> Did you mean "at least"?

No, I meant "at most". (This is for the #1 strategy, matching the Google style -- if we go that route, we'd want to disallow the use of *combinations* of those keywords, since that'd be redundant and would reduce the ability of the compiler to be strict.  In that scenario, people should be using *at most* one of the keywords -- or none of them, for non-virtual functions.)
I just want to be sure this minor-but-still-worth-considering review comment isn't lost, assuming we do go with the more concise coding style here:

(In reply to Daniel Holbert [:dholbert] (out of office 4/30 & 5/1) from comment #2)
> (1) This s/final override/final fixup seems to leave behind a trailing blank
> space (before the ";"), e.g. here:
> 
> >+++ b/dom/base/Element.h
> >-  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) final override;
> >+  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) final ;

ehsan: can you see about fixing this? Either via a second cleanup sed-script + patch, with a cross-tree "s/final ;/final;/" replacement, or (better) via filing & perhaps fixing upstream in clang-tidy.
Flags: needinfo?(ehsan)
(In reply to Daniel Holbert [:dholbert] from comment #12)
> I just want to be sure this minor-but-still-worth-considering review comment
> isn't lost, assuming we do go with the more concise coding style here:
> 
> (In reply to Daniel Holbert [:dholbert] (out of office 4/30 & 5/1) from
> comment #2)
> > (1) This s/final override/final fixup seems to leave behind a trailing blank
> > space (before the ";"), e.g. here:
> > 
> > >+++ b/dom/base/Element.h
> > >-  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) final override;
> > >+  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) final ;
> 
> ehsan: can you see about fixing this? Either via a second cleanup sed-script
> + patch, with a cross-tree "s/final ;/final;/" replacement, or (better) via
> filing & perhaps fixing upstream in clang-tidy.

Oh yes sorry, I wasn't ignoring it.  :-)  But I do believe that is also a bug in my modifications to clang-tidy so I'm holding on for fixing it before we decide how we want to do things.  Once we did that, I'll make sure the plugin is fixed according to this before submitting a new patch.
Flags: needinfo?(ehsan)
This patch has been automatically generated using clang-tidy's
misc-use-override check.  It adds override keywords where they were
missing, and removes them if the final keyword is also specified, in
which case override is redundant.
Attachment #8597955 - Attachment is obsolete: true
Attachment #8633289 - Flags: review?(dholbert)
Attachment #8598049 - Attachment is obsolete: true
All of the review issues brought up before were bugs in my modifications.
Comment on attachment 8633289 [details] [diff] [review]
Fix the usage of override and final keywords in the tree

Several things:

 (1) I'm not comfortable being the sole reviewer on a huge cross-tree change like this. I'd be more comfortable if you also had sign-off from an xpcom-module reviewer, or a superreviewer. (They don't necessarily need to do a thorough review, just an additional voice of "this seems fine" -- I'm don't want to step on toes by unilaterally r+'ing changes that affect modules where I'm not really involved.)

 (2) It looks like this doesn't touch any 3rd-party code (not that I noticed at least), which is great. Can you elaborate on how you achieved that, with the automated tool?

 (3) This will definitely cause bitrot in people's patches. Could you prepare a dev.platform post warning people about this? (And do you have any ideas on ways for people to unbitrot their patches? I don't imagine the automated tool runs on patch files.)

 (4) Commit-message nits:
>Bug 1158776 - Fix the usage of override and final keywords in the tree; r=dholbert

* s/override and final/'virtual', 'override', and 'final'/
* And maybe add: "so that at most one is specified in any function decl".  ("Fix" on its own is vague, & isn't quite right; the current code isn't exactly broken [though it does violate the newly-established style guide rule]
* Also might be worth linking to the google groups resolution:
  https://groups.google.com/d/msg/mozilla.dev.platform/sxYiu8JHwe4/nf1Jtftiw60J
in the later lines of the commit message. (Or just adding it to the URL field here.


 (5) One nit with the actual code:

>+++ b/dom/asmjscache/AsmJSCache.cpp
>@@ -238,17 +238,17 @@ public:
>   : mQuotaObject(nullptr),
>     mFileSize(INT64_MIN),
>     mFileDesc(nullptr),
>     mFileMap(nullptr),
>     mMappedMemory(nullptr)
>   { }
> 
>   ~FileDescriptorHolder()
>-  {
>+  override {
[...]
>   }

The fixup here ^^ is wrong -- "override" should go on the previous line, after the function-signature.  It shouldn't be inserted on a curly-brace-only line, pushing the curly-brace rightwards - that's kind of a bizarre style which I don't think we use anywhere else.

In places where this patch adds 'override', can we standardize on adding it on the same line as the closing-paren of the function-signature?
Attachment #8633289 - Flags: review?(dholbert) → feedback+
I need to figure out how to put override in the right place.  I probably have a patch for it somewhere, need to dig it up!  And yes, I'll announce on dev.platform.  Need to find another reviewer too!
Depends on: 1197185
Andi, is this something that would interest you to work on by any chance? :-)
Assignee: ehsan → nobody
Flags: needinfo?(bpostelnicu)
Isn't this something that we can automate now with every patch, using the bot? Or better yet from:
>> mach static-analysis
Since we have the auto-fix option.
Flags: needinfo?(bpostelnicu)
Depends on: 1428535
btw, I posted a mach lint check (cpp-virtual-final) in bug 1436263 for virtual function declarations with multiple specifiers. Virtual function declarations should specify only one of `virtual`, `final`, or `override`. Unfortunately, the lint doesn't warn about `virtual void Example() override` at this time because there are 8000+ instances and it doesn't warn about function declarations that span multiple lines because the regex can't match across line breaks.

A clang static analysis check would be more thorough, but this mach lint is a worse-is-better placeholder until someone steps up to write a proper clang check. :)
Depends on: 1436263
Product: Core → Firefox Build System
As we have Chris's lint, even if this isn't perfect, I think we can close this one.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Assignee: nobody → nobody
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.