Thanks for proposing this library. Scope guards are a natural and much
needed addition to Boost. The following is my review of the proposed
Boost.Scope.
The first thing I looked for in this library was a convenience SCOPE_EXIT {
... }; macro that would provide the functionality in the form of a control
flow construct. I've managed to quickly find it, but it wasn't exactly what
I was expecting.
BOOST_SCOPE_FINAL []
{
std::cout << "Hello world!" << std::endl;
};
I found it rather odd that the capture list is not included in the macro. I
looked a bit further into the documentation to find this:
BOOST_SCOPE_FINAL std::ref(cleanup_func);
Is this use case the reason why the capture list is required? It seems like
a very rare one that shouldn't pessimize the interface for 99% of use
cases. Performance shouldn't be a concern because the compiler sees all
this code and inlines it anyway. In my view, this just makes the interface
worse and less pleasant to use.
scope_success doesn't seem terribly useful. I have yet to see a legitimate
use case of this type. I haven't found a motivating example in the
documentation. There should either be one, or this type should be removed
from the library.
There is no BOOST_SCOPE_FAIL macro. Why is it missing?
I found the scope_fail example pretty bad:
class collection
{
std::set< std::shared_ptr< object > > objects;
public:
void add_object(std::shared_ptr< object > const& obj)
{
// Create a deactivated scope guard initially
std::set< std::shared_ptr< object > >::iterator it;
boost::scope::scope_fail rollback_guard{[&, this]
{
objects.erase(it);
},
false};
bool inserted;
std::tie(it, inserted) = objects.insert(obj);
if (inserted)
{
// Activate rollback guard
rollback_guard.set_active(true);
}
obj->on_added_to_collection(*this);
}
};
It could be simply rewritten to not use set_active (which should be
discouraged, because it makes code less declarative and harder to follow):
class collection
{
std::set< std::shared_ptr< object > > objects;
public:
void add_object(std::shared_ptr< object > const& obj)
{
// Create a deactivated scope guard initially
std::set< std::shared_ptr< object > >::iterator it;
bool inserted;
std::tie(it, inserted) = objects.insert(obj);
if (inserted)
{
boost::scope::scope_fail rollback_guard{[&, this]
{
objects.erase(it);
}};
obj->on_added_to_collection(*this);
}
}
};
Could be made even cleaner by using an early return: if (!inserted) return;
All destructor declarations in the documentation lack noexcept
specification, which by default means that they're noexcept, but they're
actually not if you look at the Throws section, or the implementation.
Also, the question presents itself: why are these destructors allowed to
throw? I feel like it is common knowledge today that destructors should not
throw, and this is heavily frowned upon. I can see the want to "be generic"
here, but I don't see why a library should entertain broken code.
I also noticed that certain scope guards are move constructible. Why? I
cannot imagine myself ever approving of someone moving a scope guard on
code review. Scope guards are to be used exclusively as local variables and
don't need to be moved. This just opens up room for misuse.
I don't see any compelling use cases for scope_exit::set_active in the
documentation. The only example I find motivating is this one:
void push_back(int x, std::vector<int>& vec1,
std::vector<int>& vec2)
{
vec1.push_back(x);
// Revert vec1 modification on failure
boost::scope::scope_exit rollback_guard{[&]
{
vec1.pop_back();
}};
vec2.push_back(x);
// Commit vec1 modification
rollback_guard.set_active(false);
}
This could as well have used rollback_guard.release();. This matches my
personal experience: only dismiss/release/cancel is necessary for real use
cases ("commiting" something and thus disabling the "rollback" action).
absl::Cleanup for example, supports only Cancel(). There is value in
preventing users from shooting themselves in the foot.
I don't like the name scope_exit::release personally. I feel like "dismiss"
or "cancel" would be much better. I know that std::experimental::scope_exit
calls it "release", but we shouldn't appeal to false authority.
I did not like the locked_write_string example. Reading this code locally:
// Lock the file
while (flock(fd, LOCK_EX) < 0)
{
err = errno;
if (err != EINTR)
return;
}
it's hard to get what's happening, why we're setting err and how big of an
impact that has on function behavior. A better way to write this code would
be to simply extract this piece into its own function:
err = errno;
if (err != EINTR)
return;
together with the ec = std::error_code(err, std::generic_category()), which
doesn't require a scope guard.
fd_deleter and unique_fd provide some nice handling for EINTR, which wasn't
required but I think it's nice to have and it's good that it's a part of
this library!
I think make_unique_resource_checked approaches the problem from the wrong
direction. This code:
// Create a unique resource for a file
auto file = boost::scope::make_unique_resource_checked(
open(filename.c_str(), O_CREAT | O_WRONLY)
-1,
fd_deleter());
if (!file.allocated())
{
int err = errno;
throw std::system_error(err, std::generic_category(), "Failed to
open file " + filename);
}
could be this:
// Create a unique resource for a file
int file = open(filename.c_str(), O_CREAT | O_WRONLY);
boost::scope::scope_final close_file(fd_deleter{});
if (file != -1)
{
int err = errno;
throw std::system_error(err, std::generic_category(), "Failed to
open file " + filename);
}
This has the advantage of being able to use `file` directly for the rest of
the code instead of `file.get()`.
I think a similar utility would be more useful in form of a class template,
for example:
resource