From d49495414c245aa54c700c3b87f6de9ff294a56d Mon Sep 17 00:00:00 2001 From: axgillies Date: Tue, 15 Jul 2025 16:14:25 +0200 Subject: [PATCH 1/2] Update 2018-04-20-totw-120.md Syncs Tip #120 with internal version. --- _posts/2018-04-20-totw-120.md | 80 +++++++++++++++++------------------ 1 file changed, 39 insertions(+), 41 deletions(-) diff --git a/_posts/2018-04-20-totw-120.md b/_posts/2018-04-20-totw-120.md index 857e0c10..def0cb00 100644 --- a/_posts/2018-04-20-totw-120.md +++ b/_posts/2018-04-20-totw-120.md @@ -12,13 +12,12 @@ Originally posted as TotW #120 on August 16, 2012 *by Samuel Benzaquen, [(sbenza@google.com)](mailto:sbenza@gmail.com)* -Let's suppose you have a code snippet like below, which relies on an RAII -cleanup function, which seems to be working as expected: +Let's suppose you have this code snippet, which seems to be working as expected: ```c++ -MyStatus DoSomething() { - MyStatus status; - auto log_on_error = RunWhenOutOfScope([&status] { +absl::Status DoSomething() { + absl::Status status; + auto log_on_error = absl::MakeCleanup([&status] { if (!status.ok()) LOG(ERROR) << status; }); status = DoA(); @@ -31,21 +30,19 @@ MyStatus DoSomething() { } ``` -A refactor changes the last line from `return status;` to -`return MyStatus();` and suddenly the code stopped logging errors. +A refactor changes the last line from `return status;` to `return +absl::OkStatus();` and suddenly the code stopped logging the errors. What is going on? ## Summary -Never access (read or write) the return variable of a function after the return -statement has run. Unless you take great care to do it correctly, the behavior -is unspecified. +Never access (read or write) the return variable after the return statement has +run. Unless you take great care to do it correctly, the behavior is unspecified. Return variables are implicitly accessed by destructors after they have been -copied or move (See Section 6.6.3 of the C++11 standard [stmt.return]), which -is how this unexpected access occurs, but the copy/move may be elided, which -is why behavior is undefined. +copied or moved ([stmt.return]), which is how this unexpected access occurs, but +the copy/move may be elided, which is why behavior is unspecified. This tip only applies when you are returning a non-reference local variable. Returning any other expression will not trigger this problem. @@ -53,23 +50,23 @@ Returning any other expression will not trigger this problem. ## The Problem There are 2 different optimizations on return statements that can modify the -behavior of our original code snippet: NRVO (See [TotW #11](/tips/11)) and +behavior of that code snippet: the named return value optimization (NRVO) and implicit moves. -The *before* code worked because copy elision is taking place and the -`return` statement is not actually doing any work. The variable `status` is -already constructed in the return address and the cleanup object is seeing this -unique instance of the `MyStatus` object _after_ the return statement. +The *before* code was working because NRVO is being performed and the `return` +statement is not actually doing any work. The variable `status` is already +constructed in the return address and the cleanup object is seeing this unique +instance of the `Status` object _after_ the return statement. -In the *after* code, copy elision is not being applied and the returned variable -is being moved into the return value. `RunWhenOutOfScope()` runs after the -move operation is done and it is seeing a moved-from `MyStatus` object. +In the *after* code, NRVO is not being performed and the returned variable is +being moved into the return value. The cleanup object is being run after the +move operation is done and it is seeing a moved-from `Status`. -Note that the *before* code was not correct either, as it relies on the copy -elision optimization for correctness. We encourage you to rely on copy elision -for performance (See [TotW #24](/tips/24)), but not correctness. After all, -copy elision is an _optional_ optimization and compiler options or quality of -implementation of the compiler can affect whether or not it happens. +Note that the *before* code was not correct either, as it relies on NRVO for +correctness. We encourage you to rely on NRVO for performance (See TotW #24), +but not correctness. After all, NRVO is an _optional_ optimization and compiler +options or quality of implementation of the compiler can affect whether or not +it happens. ## Solution @@ -81,8 +78,8 @@ processing, and one that calls the first one and does the post-processing (ie logging on error). Eg: ```c++ -MyStatus DoSomething() { - MyStatus status; +absl::Status DoSomething() { + absl::Status status; status = DoA(); if (!status.ok()) return status; status = DoB(); @@ -91,8 +88,9 @@ MyStatus DoSomething() { if (!status.ok()) return status; return status; } -MyStatus DoSomethingAndLog() { - MyStatus status = DoSomething(); + +absl::Status DoSomethingAndLog() { + absl::Status status = DoSomething(); if (!status.ok()) LOG(ERROR) << status; return status; } @@ -100,17 +98,17 @@ MyStatus DoSomethingAndLog() { If you are only reading the value, you could also make sure to disable the optimizations instead. That would force a copy to be made all the time and the -post-processing will not see a moved-from value. E.g.: +post-processing will not see a moved-from value. Eg: ```c++ -MyStatus DoSomething() { - MyStatus status_no_nrvo; +absl::Status DoSomething() { + absl::Status status_no_nrvo; // 'status' is a reference so NRVO and all associated optimizations // will be disabled. // The 'return status;' statements will always copy the object and Logger // will always see the correct value. - MyStatus& status = status_no_nrvo; - auto log_on_error = RunWhenOutOfScope([&status] { + absl::Status& status = status_no_nrvo; + auto log_on_error = absl::MakeCleanup([&status] { if (!status.ok()) LOG(ERROR) << status; }); status = DoA(); @@ -123,19 +121,19 @@ MyStatus DoSomething() { } ``` -## Another example: +## Another Real Life Example ```c++ std::string EncodeVarInt(int i) { std::string out; - StringOutputStream string_output(&out); - CodedOutputStream coded_output(&string_output); + proto2::io::StringOutputStream string_output(&out); + proto2::io::CodedOutputStream coded_output(&string_output); coded_output.WriteVarint32(i); return out; } ``` -`CodedOutputStream` does some work in the destructor to trim unused trailing +`CodedOutputStream` does some work on the destructor to trim unused trailing bytes. This function can leave garbage bytes on the string if NRVO does not happen. @@ -149,8 +147,8 @@ the block is finished. Like this: std::string EncodeVarInt(int i) { std::string out; { - StringOutputStream string_output(&out); - CodedOutputStream coded_output(&string_output); + proto2::io::StringOutputStream string_output(&out); + proto2::io::CodedOutputStream coded_output(&string_output); coded_output.WriteVarint32(i); } // At this point the streams are destroyed and they already flushed. From 1e5eb2d0a28b4e6635cece2108e87983924eb2e8 Mon Sep 17 00:00:00 2001 From: axgillies Date: Sun, 20 Jul 2025 09:10:09 +0200 Subject: [PATCH 2/2] Update 2018-04-20-totw-120.md --- _posts/2018-04-20-totw-120.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/_posts/2018-04-20-totw-120.md b/_posts/2018-04-20-totw-120.md index def0cb00..9551da5f 100644 --- a/_posts/2018-04-20-totw-120.md +++ b/_posts/2018-04-20-totw-120.md @@ -10,7 +10,7 @@ order: "120" Originally posted as TotW #120 on August 16, 2012 -*by Samuel Benzaquen, [(sbenza@google.com)](mailto:sbenza@gmail.com)* +*by Samuel Benzaquen, [(sbenza@google.com)](mailto:sbenza@google.com)* Let's suppose you have this code snippet, which seems to be working as expected: