[clang-tidy] Run modernize-use-override over JSC
Created attachment 399053 [details] Patch
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 on attachment 399053 [details] Patch This doesn’t follow the WebKit project guideline of using "final" rather than "override" whenever possible.
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.
(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...
(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.
Created attachment 399071 [details] Patch
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.
Created attachment 399078 [details] Patch
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.
(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.
(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).
Committed r261542: <https://trac.webkit.org/changeset/261542> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399078 [details].
<rdar://problem/63117947>
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?
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.
Reverted r261542 for reason: Broke internal builds Committed r261556: <https://trac.webkit.org/changeset/261556>
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.
(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 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".
Created attachment 399145 [details] Patch for landing
(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.
> 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?
(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.
Committed r261567: <https://trac.webkit.org/changeset/261567> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399145 [details].
(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.