Skip to content

Update TotW #120 to reflect the OSS availability of absl::Cleanup and absl::Status #501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 24 additions & 23 deletions _posts/2018-04-20-totw-120.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,17 @@ order: "120"

Originally posted as TotW #120 on August 16, 2012

*by Samuel Benzaquen, [([email protected])](mailto:sbenza@gmail.com)*
_by Samuel Benzaquen, [([email protected])](mailto:sbenza@google.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 a code snippet like below, which relies on a scope
guard utility, which seems to be working as expected:

```c++
MyStatus DoSomething() {
MyStatus status;
auto log_on_error = RunWhenOutOfScope([&status] {
absl::Status DoSomething() {
absl::Status status;
absl::Cleanup log_on_error = [&status] {
if (!status.ok()) LOG(ERROR) << status;
});
};
status = DoA();
if (!status.ok()) return status;
status = DoB();
Expand All @@ -32,7 +32,8 @@ MyStatus DoSomething() {
```

A refactor changes the last line from `return status;` to
`return MyStatus();` and suddenly the code stopped logging errors.
`return absl::OkStatus();` and suddenly the code stopped logging
errors.

What is going on?

Expand All @@ -56,16 +57,16 @@ 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
implicit moves.

The *before* code worked because copy elision is taking place and the
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.
unique instance of the `absl::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, copy elision is not being applied and the returned variable
is being moved into the return value. `absl::Cleanup` runs after the move
operation is done and it is seeing a moved-from `absl::Status` object.

Note that the *before* code was not correct either, as it relies on the copy
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
Expand All @@ -81,8 +82,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();
Expand All @@ -91,8 +92,8 @@ 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;
}
Expand All @@ -103,16 +104,16 @@ 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.:

```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;
absl::Cleanup log_on_error = [&status] {
if (!status.ok()) LOG(ERROR) << status;
});
};
status = DoA();
if (!status.ok()) return status;
status = DoB();
Expand Down