Bug 211743 – [clang-tidy] Run modernize-use-override over JSC, then ensure as much as possible is final

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

[clang-tidy] Run modernize-use-override over JSC

[Ross Kirsling]

Created attachment 399053 [details]
Patch

[Ross Kirsling]

Looks like most of these are marking destructors in derived classes, but there are a handful of nice corrections for where `virtual` was used when `override` was intended.

[Darin Adler]

Comment on attachment 399053 [details]
Patch

This doesn’t follow the WebKit project guideline of using "final" rather than "override" whenever possible.

[Darin Adler]

I’m not sure I love that guideline, but I don’t want a tool to change that guideline for us! We should decide first.

[Ross Kirsling]

(In reply to Darin Adler from comment #3)
> Comment on attachment 399053 [details]
> Patch
> 
> This doesn’t follow the WebKit project guideline of using "final" rather
> than "override" whenever possible.

Hmm, I was thinking it would do so if the class itself was marked final. I can go over the cases individually to make sure...

[Darin Adler]

(In reply to Ross Kirsling from comment #5)
> Hmm, I was thinking it would do so if the class itself was marked final. I
> can go over the cases individually to make sure...

JITThunks.h shows that it does not. Many of these classes *could* be marked final.

[Ross Kirsling]

Created attachment 399071 [details]
Patch

[Ross Kirsling]

There doesn't seem to be a way to automate making things `final`, but this goes through all the classes which had `override` corrections and makes as many of them `final` as possible.

[Ross Kirsling]

Created attachment 399078 [details]
Patch

[Yusuke Suzuki]

I like that. This is particularly useful for JSObject hierarchy: due to IsoSubspace etc. we need to be extra careful about the hierarchy of JSObject. This `final` makes it explicit that this class is final class. This is really good for readability.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #10)
> I like that. This is particularly useful for JSObject hierarchy: due to
> IsoSubspace etc. we need to be extra careful about the hierarchy of
> JSObject. This `final` makes it explicit that this class is final class.
> This is really good for readability.

And because this is driven by tooling, we can rather consider that the given class A could have derived class if it does not have `final` annotation.

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #11)
> (In reply to Yusuke Suzuki from comment #10)
> > I like that. This is particularly useful for JSObject hierarchy: due to
> > IsoSubspace etc. we need to be extra careful about the hierarchy of
> > JSObject. This `final` makes it explicit that this class is final class.
> > This is really good for readability.
> 
> And because this is driven by tooling, we can rather consider that the given
> class A could have derived class if it does not have `final` annotation.

It's definitely a nice step in the right direction, but I guess we'd need to write our own rule if we wanted to statically analyze "classes which should be final". Going through every class in JSC by hand is a bit much, but I could probably go through the rest of the `override` cases (because in this patch, I just touched the files that clang-tidy had already touched).

[EWS]

Committed r261542: <https://trac.webkit.org/changeset/261542>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399078 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63117947>

[Alexey Proskuryakov]

Comment on attachment 399078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399078&action=review

> Source/JavaScriptCore/API/JSScriptRef.cpp:49
> -struct OpaqueJSScript : public SourceProvider {
> +struct OpaqueJSScript final : public SourceProvider {
>  public:
>      static WTF::Ref<OpaqueJSScript> create(VM& vm, const SourceOrigin& sourceOrigin, URL&& url, int startingLineNumber, const String& source)
>      {
>          return WTF::adoptRef(*new OpaqueJSScript(vm, sourceOrigin, WTFMove(url), startingLineNumber, source));
>      }
>  
> -    unsigned hash() const override
> +    unsigned hash() const final

What is the point of having final functions in final classes?

[Alexey Proskuryakov]

This somehow broke internal build, seemingly with newer clang versions. I don't see how, but build failure history is clear.

Undefined symbols for architecture arm64:
  "JSC::B3::PatchpointSpecial::~PatchpointSpecial()", referenced from:
      std::__1::default_delete<JSC::B3::PatchpointSpecial>::operator()(JSC::B3::PatchpointSpecial*) const in testair.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Not just arm64, all platforms.

[Truitt Savell]

Reverted r261542 for reason:

Broke internal builds

Committed r261556: <https://trac.webkit.org/changeset/261556>

[Saam Barati]

Comment on attachment 399078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399078&action=review

> Source/JavaScriptCore/b3/B3PatchpointSpecial.h:47
> +    ~PatchpointSpecial() final;

Internal Apple builds are failing. I think this needs to be exported too.

[Saam Barati]

(In reply to Saam Barati from comment #18)
> Comment on attachment 399078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399078&action=review
> 
> > Source/JavaScriptCore/b3/B3PatchpointSpecial.h:47
> > +    ~PatchpointSpecial() final;
> 
> Internal Apple builds are failing. I think this needs to be exported too.

Seems like this was pointed out.

My guess is this turns into a direct call to the destructor in some places instead of a virtual call

[Darin Adler]

Comment on attachment 399078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399078&action=review

>> Source/JavaScriptCore/API/JSScriptRef.cpp:49
>> +    unsigned hash() const final
> 
> What is the point of having final functions in final classes?

In a class marked final, there are multiple ways to annotate overriding functions. We could use "override", "final", "override final", or "final override" and all would be equivalent. When we noticed this we had a conversation about which to choose on the webkit-dev mailing list. We selected "final".

[Ross Kirsling]

Created attachment 399145 [details]
Patch for landing

[Ross Kirsling]

(In reply to Saam Barati from comment #19)
> My guess is this turns into a direct call to the destructor in some places
> instead of a virtual call

Yeah, EWS saw this happen in a couple of places in the patch that follows this (bug 211772) too.

[Alexey Proskuryakov]

> In a class marked final, there are multiple ways to annotate overriding
> functions. We could use "override", "final", "override final", or "final
> override" and all would be equivalent.

No annotation would be equivalent too, correct?

> When we noticed this we had a
> conversation about which to choose on the webkit-dev mailing list. We
> selected "final".

I see. I forgot that discussion because it didn't feel like there was any decision made, and it certainly isn't in the style guide. This option seems like the strangest one to me, as it's most obviously unnecessary. Is the purpose of adding an annotation to make it clear that the function is virtual, or something else?

[Darin Adler]

(In reply to Alexey Proskuryakov from comment #23)
> > In a class marked final, there are multiple ways to annotate overriding
> > functions. We could use "override", "final", "override final", or "final
> > override" and all would be equivalent.
> 
> No annotation would be equivalent too, correct?

No. Without annotation, we would *not* get a compilation error if it’s not actually overriding something. I believe the thing that these all have in common is that they all *do* provide that error.

[EWS]

Committed r261567: <https://trac.webkit.org/changeset/261567>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399145 [details].

[Darin Adler]

(In reply to Alexey Proskuryakov from comment #23)
> I see. I forgot that discussion because it didn't feel like there was any
> decision made

OK. I remember a decision made.

> it certainly isn't in the style guide

We can fix that.

> Is the
> purpose of adding an annotation to make it clear that the function is
> virtual, or something else?

It’s to make it clear that the function is overriding something, not a newly introduced function.