From 604d5f5bcae5f89778acf416334a823bcb114402 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Thu, 9 Feb 2023 13:27:43 -0500 Subject: [PATCH] AK+Everywhere: Do not implicitly copy variables in TRY macros For example, consider cases where we want to propagate errors only in specific instances: auto result = read_data(); // something like ErrorOr if (result.is_error() && result.error().code() != EINTR) continue; auto bytes = TRY(result); The TRY invocation will currently copy the byte buffer when the expression (in this case, just a local variable) is stored into _temporary_result. This patch binds the expression to a reference to prevent such copies. In less trival invocations (such as TRY(some_function()), this will incur only temporary lifetime extensions, i.e. no functional change. --- AK/Try.h | 4 ++-- Kernel/Net/Socket.h | 2 +- .../Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp | 2 +- Userland/Libraries/LibAudio/LoaderError.h | 2 +- Userland/Libraries/LibJS/Runtime/Completion.h | 4 ++-- Userland/Libraries/LibJS/Runtime/PromiseCapability.h | 4 ++-- Userland/Libraries/LibVideo/DecoderError.h | 2 +- Userland/Libraries/LibVideo/PlaybackManager.cpp | 4 ++-- Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp | 2 +- Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp | 2 +- Userland/Utilities/matroska.cpp | 2 +- 11 files changed, 15 insertions(+), 15 deletions(-) diff --git a/AK/Try.h b/AK/Try.h index b3dff1b273..9adf1c576d 100644 --- a/AK/Try.h +++ b/AK/Try.h @@ -25,7 +25,7 @@ ({ \ /* Ignore -Wshadow to allow nesting the macro. */ \ AK_IGNORE_DIAGNOSTIC("-Wshadow", \ - auto _temporary_result = (expression)); \ + auto&& _temporary_result = (expression)); \ static_assert(!::AK::Detail::IsLvalueReference, \ "Do not return a reference from a fallible expression"); \ if (_temporary_result.is_error()) [[unlikely]] \ @@ -37,7 +37,7 @@ ({ \ /* Ignore -Wshadow to allow nesting the macro. */ \ AK_IGNORE_DIAGNOSTIC("-Wshadow", \ - auto _temporary_result = (expression)); \ + auto&& _temporary_result = (expression)); \ static_assert(!::AK::Detail::IsLvalueReference, \ "Do not return a reference from a fallible expression"); \ VERIFY(!_temporary_result.is_error()); \ diff --git a/Kernel/Net/Socket.h b/Kernel/Net/Socket.h index 2599b3d6db..182a06bc5b 100644 --- a/Kernel/Net/Socket.h +++ b/Kernel/Net/Socket.h @@ -188,7 +188,7 @@ private: // This is a special variant of TRY() that also updates the socket's SO_ERROR field on error. #define SOCKET_TRY(expression) \ ({ \ - auto result = (expression); \ + auto&& result = (expression); \ if (result.is_error()) \ return set_so_error(result.release_error()); \ static_assert(!::AK::Detail::IsLvalueReference, \ diff --git a/Meta/Lagom/Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp b/Meta/Lagom/Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp index cace43b441..e7b2880cb4 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp @@ -233,7 +233,7 @@ struct CLDR { // with locales such as "en-GB-oed" that are canonically invalid locale IDs. #define TRY_OR_DISCARD(expression) \ ({ \ - auto _temporary_result = (expression); \ + auto&& _temporary_result = (expression); \ if (_temporary_result.is_error()) \ return; \ static_assert(!::AK::Detail::IsLvalueReference, \ diff --git a/Userland/Libraries/LibAudio/LoaderError.h b/Userland/Libraries/LibAudio/LoaderError.h index a54a048902..9f31b6afad 100644 --- a/Userland/Libraries/LibAudio/LoaderError.h +++ b/Userland/Libraries/LibAudio/LoaderError.h @@ -68,7 +68,7 @@ struct LoaderError { // Convenience TRY-like macro to convert an Error to a LoaderError #define LOADER_TRY(expression) \ ({ \ - auto _temporary_result = (expression); \ + auto&& _temporary_result = (expression); \ if (_temporary_result.is_error()) \ return LoaderError(_temporary_result.release_error()); \ static_assert(!::AK::Detail::IsLvalueReference, \ diff --git a/Userland/Libraries/LibJS/Runtime/Completion.h b/Userland/Libraries/LibJS/Runtime/Completion.h index 294938a8e6..7fe307bf01 100644 --- a/Userland/Libraries/LibJS/Runtime/Completion.h +++ b/Userland/Libraries/LibJS/Runtime/Completion.h @@ -21,7 +21,7 @@ namespace JS { ({ \ /* Ignore -Wshadow to allow nesting the macro. */ \ AK_IGNORE_DIAGNOSTIC("-Wshadow", \ - auto _temporary_result = (expression)); \ + auto&& _temporary_result = (expression)); \ if (_temporary_result.is_error()) { \ VERIFY(_temporary_result.error().code() == ENOMEM); \ return vm.throw_completion(JS::ErrorType::OutOfMemory); \ @@ -35,7 +35,7 @@ namespace JS { ({ \ /* Ignore -Wshadow to allow nesting the macro. */ \ AK_IGNORE_DIAGNOSTIC("-Wshadow", \ - auto _temporary_result = (expression)); \ + auto&& _temporary_result = (expression)); \ if (_temporary_result.is_error()) { \ auto _completion = _temporary_result.release_error(); \ \ diff --git a/Userland/Libraries/LibJS/Runtime/PromiseCapability.h b/Userland/Libraries/LibJS/Runtime/PromiseCapability.h index 41c4b81170..a5eb801f48 100644 --- a/Userland/Libraries/LibJS/Runtime/PromiseCapability.h +++ b/Userland/Libraries/LibJS/Runtime/PromiseCapability.h @@ -43,7 +43,7 @@ private: // 27.2.1.1.1 IfAbruptRejectPromise ( value, capability ), https://tc39.es/ecma262/#sec-ifabruptrejectpromise #define __TRY_OR_REJECT(vm, capability, expression, CALL_CHECK) \ ({ \ - auto _temporary_try_or_reject_result = (expression); \ + auto&& _temporary_try_or_reject_result = (expression); \ /* 1. If value is an abrupt completion, then */ \ if (_temporary_try_or_reject_result.is_error()) { \ /* a. Perform ? Call(capability.[[Reject]], undefined, « value.[[Value]] »). */ \ @@ -69,7 +69,7 @@ private: // 27.2.1.1.1 IfAbruptRejectPromise ( value, capability ), https://tc39.es/ecma262/#sec-ifabruptrejectpromise #define TRY_OR_REJECT_WITH_VALUE(vm, capability, expression) \ ({ \ - auto _temporary_try_or_reject_result = (expression); \ + auto&& _temporary_try_or_reject_result = (expression); \ /* 1. If value is an abrupt completion, then */ \ if (_temporary_try_or_reject_result.is_error()) { \ /* a. Perform ? Call(capability.[[Reject]], undefined, « value.[[Value]] »). */ \ diff --git a/Userland/Libraries/LibVideo/DecoderError.h b/Userland/Libraries/LibVideo/DecoderError.h index b377b2552a..c66b7717ea 100644 --- a/Userland/Libraries/LibVideo/DecoderError.h +++ b/Userland/Libraries/LibVideo/DecoderError.h @@ -79,7 +79,7 @@ private: #define DECODER_TRY(category, expression) \ ({ \ - auto _result = ((expression)); \ + auto&& _result = ((expression)); \ if (_result.is_error()) [[unlikely]] { \ auto _error_string = _result.release_error().string_literal(); \ return DecoderError::from_source_location( \ diff --git a/Userland/Libraries/LibVideo/PlaybackManager.cpp b/Userland/Libraries/LibVideo/PlaybackManager.cpp index b3953a01a0..0024fd4a0f 100644 --- a/Userland/Libraries/LibVideo/PlaybackManager.cpp +++ b/Userland/Libraries/LibVideo/PlaybackManager.cpp @@ -15,7 +15,7 @@ namespace Video { #define TRY_OR_FATAL_ERROR(expression) \ ({ \ - auto _fatal_expression = (expression); \ + auto&& _fatal_expression = (expression); \ if (_fatal_expression.is_error()) { \ dispatch_fatal_error(_fatal_expression.release_error()); \ return; \ @@ -156,7 +156,7 @@ bool PlaybackManager::decode_and_queue_one_sample() #define TRY_OR_ENQUEUE_ERROR(expression) \ ({ \ - auto _temporary_result = ((expression)); \ + auto&& _temporary_result = ((expression)); \ if (_temporary_result.is_error()) { \ dbgln_if(PLAYBACK_MANAGER_DEBUG, "Enqueued decoder error: {}", _temporary_result.error().string_literal()); \ m_frame_queue->enqueue(FrameQueueItem::error_marker(_temporary_result.release_error())); \ diff --git a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp index 37f0ee1d7e..33561de997 100644 --- a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp @@ -44,7 +44,7 @@ namespace Web::Fetch::Fetching { #define TRY_OR_IGNORE(expression) \ ({ \ - auto _temporary_result = (expression); \ + auto&& _temporary_result = (expression); \ if (_temporary_result.is_error()) \ return; \ static_assert(!::AK::Detail::IsLvalueReference, \ diff --git a/Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp b/Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp index c9da79077f..a2412ac5c0 100644 --- a/Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp +++ b/Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp @@ -33,7 +33,7 @@ namespace Web::WebDriver { #define TRY_OR_JS_ERROR(expression) \ ({ \ - auto _temporary_result = (expression); \ + auto&& _temporary_result = (expression); \ if (_temporary_result.is_error()) [[unlikely]] \ return ExecuteScriptResultType::JavaScriptError; \ static_assert(!::AK::Detail::IsLvalueReference, \ diff --git a/Userland/Utilities/matroska.cpp b/Userland/Utilities/matroska.cpp index bd36e3809e..a79cac79f9 100644 --- a/Userland/Utilities/matroska.cpp +++ b/Userland/Utilities/matroska.cpp @@ -12,7 +12,7 @@ #define TRY_PARSE(expression) \ ({ \ - auto _temporary_result = ((expression)); \ + auto&& _temporary_result = ((expression)); \ if (_temporary_result.is_error()) [[unlikely]] { \ outln("Encountered a parsing error: {}", _temporary_result.error().string_literal()); \ return Error::from_string_literal("Failed to parse :("); \