Bug 211743 - [clang-tidy] Run modernize-use-override over JSC, then ensure as much as possible is final
Summary: [clang-tidy] Run modernize-use-override over JSC, then ensure as much as poss...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-11 13:50 PDT by Ross Kirsling
Modified: 2020-05-12 12:08 PDT (History)
12 users (show)

See Also:


Attachments
Patch (46.37 KB, patch)
2020-05-11 13:51 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (82.46 KB, patch)
2020-05-11 17:24 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (82.33 KB, patch)
2020-05-11 17:47 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (82.37 KB, patch)
2020-05-12 11:12 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2020-05-11 13:50:53 PDT
[clang-tidy] Run modernize-use-override over JSC
Comment 1 Ross Kirsling 2020-05-11 13:51:42 PDT
Created attachment 399053 [details]
Patch
Comment 2 Ross Kirsling 2020-05-11 13:54:09 PDT
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.
Comment 3 Darin Adler 2020-05-11 13:55:49 PDT
Comment on attachment 399053 [details]
Patch

This doesn’t follow the WebKit project guideline of using "final" rather than "override" whenever possible.
Comment 4 Darin Adler 2020-05-11 13:56:45 PDT
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.
Comment 5 Ross Kirsling 2020-05-11 13:57:01 PDT
(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...
Comment 6 Darin Adler 2020-05-11 13:59:27 PDT
(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.
Comment 7 Ross Kirsling 2020-05-11 17:24:48 PDT
Created attachment 399071 [details]
Patch
Comment 8 Ross Kirsling 2020-05-11 17:40:30 PDT
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.
Comment 9 Ross Kirsling 2020-05-11 17:47:13 PDT
Created attachment 399078 [details]
Patch
Comment 10 Yusuke Suzuki 2020-05-11 18:32:33 PDT
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.
Comment 11 Yusuke Suzuki 2020-05-11 18:33:45 PDT
(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.
Comment 12 Ross Kirsling 2020-05-11 19:46:42 PDT
(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).
Comment 13 EWS 2020-05-11 20:04:16 PDT
Committed r261542: <https://trac.webkit.org/changeset/261542>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399078 [details].
Comment 14 Radar WebKit Bug Importer 2020-05-11 20:05:31 PDT
<rdar://problem/63117947>
Comment 15 Alexey Proskuryakov 2020-05-12 09:02:55 PDT
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?
Comment 16 Alexey Proskuryakov 2020-05-12 09:11:52 PDT
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.
Comment 17 Truitt Savell 2020-05-12 09:12:30 PDT
Reverted r261542 for reason:

Broke internal builds

Committed r261556: <https://trac.webkit.org/changeset/261556>
Comment 18 Saam Barati 2020-05-12 09:23:02 PDT
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.
Comment 19 Saam Barati 2020-05-12 09:24:12 PDT
(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
Comment 20 Darin Adler 2020-05-12 10:45:37 PDT
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".
Comment 21 Ross Kirsling 2020-05-12 11:12:42 PDT
Created attachment 399145 [details]
Patch for landing
Comment 22 Ross Kirsling 2020-05-12 11:16:13 PDT
(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.
Comment 23 Alexey Proskuryakov 2020-05-12 11:31:02 PDT
> 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?
Comment 24 Darin Adler 2020-05-12 11:38:21 PDT
(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.
Comment 25 EWS 2020-05-12 11:48:08 PDT
Committed r261567: <https://trac.webkit.org/changeset/261567>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399145 [details].
Comment 26 Darin Adler 2020-05-12 12:08:42 PDT
(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.