From 9feb1ce39f0389504519779a6901e8a424c1f12e Mon Sep 17 00:00:00 2001 From: Daniel Bertalan Date: Wed, 26 Jul 2023 19:02:37 +0200 Subject: [PATCH] LibJS+LibWeb: Apply the Rule of Zero to `{Nonnull,}GCPtr` The compiler-generated copy constructor and copy assignment operator already do the right thing (which is to simply copy the underlying pointer). The [Itanium C++ ABI][1] treats any class with non-trivial copy/move constructors and destructors as non-trivial for the purposes of calls -- even if they are functionally identical to the compiler-generated ones. If a class is non-trivial, it cannot be passed or returned in registers, only via an invisible reference, which is worse for codegen. This commit makes `{Nonnull,}GCPtr` trivial. As the compiler can be sure that capturing a `GCPtr` by value has no side effects, a few `-Wunused-lambda-capture` warnings had to be addressed in LibWeb. GCC seems to have a bug that prevents `ExceptionOr>>` from being implicitly constructed from `GCPtr` after this change. A non-invasive workaround is to explicitly construct the inner Variant type. [1]: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#non-trivial --- Userland/Libraries/LibJS/Heap/GCPtr.h | 20 +------------------ .../LibWeb/HTML/Scripting/Fetching.cpp | 2 +- .../LibWeb/Streams/AbstractOperations.cpp | 8 ++++---- .../LibWeb/Streams/ReadableStream.cpp | 4 ++-- 4 files changed, 8 insertions(+), 26 deletions(-) diff --git a/Userland/Libraries/LibJS/Heap/GCPtr.h b/Userland/Libraries/LibJS/Heap/GCPtr.h index 3ab7e54994..83f67caefd 100644 --- a/Userland/Libraries/LibJS/Heap/GCPtr.h +++ b/Userland/Libraries/LibJS/Heap/GCPtr.h @@ -31,11 +31,6 @@ public: { } - NonnullGCPtr(NonnullGCPtr const& other) - : m_ptr(other.ptr()) - { - } - template NonnullGCPtr(NonnullGCPtr const& other) requires(IsConvertible) @@ -43,12 +38,6 @@ public: { } - NonnullGCPtr& operator=(NonnullGCPtr const& other) - { - m_ptr = other.ptr(); - return *this; - } - template NonnullGCPtr& operator=(NonnullGCPtr const& other) requires(IsConvertible) @@ -88,7 +77,7 @@ private: template class GCPtr { public: - GCPtr() = default; + constexpr GCPtr() = default; GCPtr(T& ptr) : m_ptr(&ptr) @@ -100,11 +89,6 @@ public: { } - GCPtr(GCPtr const& other) - : m_ptr(other.ptr()) - { - } - template GCPtr(GCPtr const& other) requires(IsConvertible) @@ -129,8 +113,6 @@ public: { } - GCPtr& operator=(GCPtr const&) = default; - template GCPtr& operator=(GCPtr const& other) requires(IsConvertible) diff --git a/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp b/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp index 57e4c2471d..d7094a1744 100644 --- a/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp +++ b/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp @@ -256,7 +256,7 @@ WebIDL::ExceptionOr fetch_classic_script(JS::NonnullGCPtrunsafe_response(); diff --git a/Userland/Libraries/LibWeb/Streams/AbstractOperations.cpp b/Userland/Libraries/LibWeb/Streams/AbstractOperations.cpp index 323e31a64a..85828af4fb 100644 --- a/Userland/Libraries/LibWeb/Streams/AbstractOperations.cpp +++ b/Userland/Libraries/LibWeb/Streams/AbstractOperations.cpp @@ -868,7 +868,7 @@ WebIDL::ExceptionOr set_up_readable_stream_default_controller_from_underly // 7. If underlyingSourceDict["cancel"] exists, then set cancelAlgorithm to an algorithm which takes an argument reason and returns the result of invoking underlyingSourceDict["cancel"] with argument list « reason » and callback this value underlyingSource. if (underlying_source.cancel) { - cancel_algorithm = [&realm, controller, underlying_source_value, callback = underlying_source.cancel](auto const& reason) -> WebIDL::ExceptionOr> { + cancel_algorithm = [&realm, underlying_source_value, callback = underlying_source.cancel](auto const& reason) -> WebIDL::ExceptionOr> { // Note: callback return a promise, so invoke_callback will never return an abrupt completion auto result = MUST_OR_THROW_OOM(WebIDL::invoke_callback(*callback, underlying_source_value, reason)).release_value(); return WebIDL::create_resolved_promise(realm, result); @@ -2528,7 +2528,7 @@ WebIDL::ExceptionOr set_up_writable_stream_default_controller_from_underly // 8. If underlyingSinkDict["close"] exists, then set closeAlgorithm to an algorithm which returns the result of invoking underlyingSinkDict["close"] with argument list «» and callback this value underlyingSink. if (underlying_sink.close) { - close_algorithm = [&realm, controller, underlying_sink_value, callback = underlying_sink.close]() -> WebIDL::ExceptionOr> { + close_algorithm = [&realm, underlying_sink_value, callback = underlying_sink.close]() -> WebIDL::ExceptionOr> { // Note: callback return a promise, so invoke_callback will never return an abrupt completion auto result = MUST_OR_THROW_OOM(WebIDL::invoke_callback(*callback, underlying_sink_value)).release_value(); return WebIDL::create_resolved_promise(realm, result); @@ -2537,7 +2537,7 @@ WebIDL::ExceptionOr set_up_writable_stream_default_controller_from_underly // 9. If underlyingSinkDict["abort"] exists, then set abortAlgorithm to an algorithm which takes an argument reason and returns the result of invoking underlyingSinkDict["abort"] with argument list « reason » and callback this value underlyingSink. if (underlying_sink.abort) { - abort_algorithm = [&realm, controller, underlying_sink_value, callback = underlying_sink.abort](JS::Value reason) -> WebIDL::ExceptionOr> { + abort_algorithm = [&realm, underlying_sink_value, callback = underlying_sink.abort](JS::Value reason) -> WebIDL::ExceptionOr> { // Note: callback return a promise, so invoke_callback will never return an abrupt completion auto result = MUST_OR_THROW_OOM(WebIDL::invoke_callback(*callback, underlying_sink_value, reason)).release_value(); return WebIDL::create_resolved_promise(realm, result); @@ -3342,7 +3342,7 @@ WebIDL::ExceptionOr set_up_readable_byte_stream_controller_from_underlying // 7. If underlyingSourceDict["cancel"] exists, then set cancelAlgorithm to an algorithm which takes an argument reason and returns the result of invoking underlyingSourceDict["cancel"] with argument list « reason » and callback this value underlyingSource. if (underlying_source_dict.cancel) { - cancel_algorithm = [&realm, controller, underlying_source, callback = underlying_source_dict.cancel](auto const& reason) -> WebIDL::ExceptionOr> { + cancel_algorithm = [&realm, underlying_source, callback = underlying_source_dict.cancel](auto const& reason) -> WebIDL::ExceptionOr> { // Note: callback return a promise, so invoke_callback will never return an abrupt completion auto result = MUST_OR_THROW_OOM(WebIDL::invoke_callback(*callback, underlying_source, reason)).release_value(); return WebIDL::create_resolved_promise(realm, result); diff --git a/Userland/Libraries/LibWeb/Streams/ReadableStream.cpp b/Userland/Libraries/LibWeb/Streams/ReadableStream.cpp index affbbfad90..0c32ee8fdd 100644 --- a/Userland/Libraries/LibWeb/Streams/ReadableStream.cpp +++ b/Userland/Libraries/LibWeb/Streams/ReadableStream.cpp @@ -97,13 +97,13 @@ WebIDL::ExceptionOr ReadableStream::get_reader(ReadableStr { // 1. If options["mode"] does not exist, return ? AcquireReadableStreamDefaultReader(this). if (!options.mode.has_value()) - return TRY(acquire_readable_stream_default_reader(*this)); + return ReadableStreamReader { TRY(acquire_readable_stream_default_reader(*this)) }; // 2. Assert: options["mode"] is "byob". VERIFY(*options.mode == Bindings::ReadableStreamReaderMode::Byob); // 3. Return ? AcquireReadableStreamBYOBReader(this). - return TRY(acquire_readable_stream_byob_reader(*this)); + return ReadableStreamReader { TRY(acquire_readable_stream_byob_reader(*this)) }; } JS::ThrowCompletionOr ReadableStream::initialize(JS::Realm& realm)