Re: [scope] Scope review starts on November 26th

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2023-11-30 17:49:50

On 11/30/23 12:29, Дмитрий Архипов via Boost wrote:
> A few more comments from Reddit
> #1, a min-review:
> 
> 1. It's rather telling that scope guards and descriptor/resources
> headers are mutually independent. This library should be split into
> two.

I'll reply separately.

> 2. Why do you dispatch between std/boost type traits? Why not always
> use boost ones (which I assume already dispatch to std internally when
> possible)?

Boost.TypeTraits do not use std type traits internally, at least not at
this point. Boost.Scope uses std type traits that are available at the
given C++ level with the compiler being used, and uses Boost.TypeTraits
or self-implemented alternatives where the standard type traits are
missing. In a conforming C++17 compiler, Boost.Scope should fully rely
on std type traits.

> #2, a code review:
> 1. Hard complaint about wrappers around basic type traits like
> std::conjunction. That's a terrible anti-pattern. Just depend on the
> version of C++ that has the features you need. This is how most of the
> libs in boost depend (or did until very recently) on C++03, and as a
> result use macro-preprocessor hacks to implement variadic templates.
> If you really need to do things this way for some reason, then
> implement a deprecation policy that you'll stick to for when you'll
> remove these wrappers.

I don't think requiring C++17 at the minimum (which is what is needed to
fully rely on std type traits) is reasonable at this point in time. As
the library maintainer, I have no problem using Boost.TypeTraits, where
needed. If users do not want the library to use Boost.TypeTraits for
some reason, they can simply use C++17 mode in their compilers.

> 2. I'm not a fan of include/boost/scope/detail/header.hpp Suppressing
> warnings instead of fixing them (Unless the warnings are determined by
> the author to be caused by a compiler bug, which i've had happen
> before) should be the last approach taken.

header.hpp disables warnings that I consider not useful. I will not be
crippling the actual code trying to work around these warnings (where at
all possible) as this would not improve the code quality, IMO.

Note that the warnings are only disabled for the library code, as there
is an accompanying footer.hpp that restores the warnings to the previous
state.

If you really want to enable the warnings for some reason, you can
define BOOST_SCOPE_ENABLE_WARNINGS. No, it doesn't mean I will be fixing
them in the library code, if you do.

Also, and this relates to some further comments below, everything in
detail is library implementation details and not intended for user
consumption. Reviewing that code is fine and welcome, but keep in mind
that these components are only as "polished" as needed for the library
itself.

> 3. #ifdef BOOST_HAS_PRAGMA_ONCE : why bother?

Why not?

> 4. include/boost/scope/detail/move_or_copy_construct_ref.hpp the _t
> version of type traits are frequently less expensive for the compiler
> instantiate than the struct version. I recommend flipping these so
> that the struct version of the type-trait is derived from the _t
> version.

If you mean type aliases a-la std::remove_cv_t vs. std::remove_cv then
no, the _t versions are not less expensive and are actually implemented
through the structs. The _v type traits could potentially circumvent the
structs and use compiler intrinsics directly, but I actually don't see
this e.g. in libstdc++11. But anyway, variable templates and the _t
versions of std type traits are C++14 and my target is C++11.

> 5. I see the frequent use of typedef. Why? Are you trying to support
> C++03? I strongly recommend against supporting C++03. Use using
> instead.

I don't see much difference between them, and I'm just used to typedef.
I can change it if reviewers demand.

> 6. include/boost/scope/detail/is_not_like.hpp: no human readable
> explanation of what this type-trait is supposed to do.

It tests whether the left-hand type is not an instantiation of the
right-hand template.

> 7. include/boost/scope/detail/compact_storage.hpp : surely this should
> live in https://www.boost.org/doc/libs/1_83_0/libs/utility/doc/html/utility/utilities.html
> and not in scope ?

Maybe. If there is demand, I may move that utility to some common place
in the future.

> 8. include/boost/scope/detail/compact_storage.hpp: If you're going to
> change the implementation of things based on which version of C++ is
> being compiled with (e.g. the type traits stuff), you might as well
> provide an implementation of this with [[no_unique_address]].

I don't see the advantage of that, given that the current approach
works. Is there a use case that would specifically benefit from
[[no_unique_address]]?

> 9. include/boost/scope/detail/compact_storage.hpp: More explanation in
> the class implementation of how it works and why would be helpful for
> future readers.

This is an implementation detail. EBO is common knowledge at this point,
I don't think there's much to explain.

> 10. include/boost/scope/detail/compact_storage.hpp: Instead of get()
> member functions, what about implicit conversion operators instead?

What would be the benefit of that?

compact_storage is not intended to impersonate the type or act like it.
It is exactly what the name says - a compact way to store the object.

> 11. include/boost/scope/unique_resource.hpp: Can't you use
> boost::is_nothrow_invocable<> for the ref_wrapper::operator() noexcept
> specification?

I've already changed that in develop. Originally, I think, that code was
written before I introduced is_nothrow_invocable.

> 12. include/boost/scope/unique_resource.hpp: What does ref_wrapper buy
> you that std::reference_wrapper doesn't? Whatever that is, document it
> in the code.

I don't want to include <functional> just for std::reference_wrapper.

> 13. include/boost/scope/unique_resource.hpp : resource_holder::get()
> functions. Why not use using resource_base::get; instead? More
> compact.

I prefer to see the actual functions that I'm defining in the class.

> 14. detail::move_or_copy_construct_ref: so far everywhere i've seen
> this used, simply replacing it with std::move() would have worked just
> as well. Why use the type-trait?

move_or_copy_construct_ref is not equivalent to std::move. The latter
would create an rvalue reference regardless of whether the move
constructor is noexcept or not, and move_or_copy_construct_ref will
produce an lvalue reference if the constructor is not noexcept. This
ensures that the source object is not invalidated if the construction
throws.

This behavior is required by
https://cplusplus.github.io/fundamentals-ts/v3.html#scopeguard.exit
paragraph 7.11.

> 15. unique_resource_data::swap_impl: std::swap(m_allocated, that.allocated);

I don't want to include <algorithm> just for std::swap.

> 16. static_cast< unique_resource_data&& >(that) : std::move() by any other name?

I don't want to include <utility> just for std::move.

> 17. unique_resource.hpp: "An lvalue reference to an object type." Why
> would someone want to do this? Seems like an easy way to foot-gun
> oneself.

Personally, I don't have a practical use case for this, but I imagine it
allows for non-copyable and non-moveable resource types. That's just
what the TS supports, so Boost.Scope supports this as well.

> 18. unique_resource.hpp: "The deleter must be copy-constructible."
> Perhaps discuss whether move-constructible matters at all.

I'm not sure what you mean here. Copy-constructible implies
move-constructible.

> 19. unique_resource.hpp: "One of the unallocated resource values can
> be considered the default. Constructing the default resource value and
> assigning it to a resource object (whether allocated or not) shall not
> throw exceptions." What is enforcing this requirement?

Nothing, this is a precondition. If it is not satisfied, unique_resource
may not behave as expected.

> 20. "// Workaround for clang < 5.0 which can't pass scope_success as a
> template template parameter from within scope_success definition" Why
> support such an old compiler?

The workaround is simple enough, so why not?

> 21. include/boost/scope/scope_success.hpp: logical_not: is this not
> just std::not_fn ? Previous to C++17, std::not1. Why implement this
> yourself?

I don't want to include <functional> just for that trivial function
object wrapper.

> 22. "Scope exit guard that invokes a function upon leaving the scope
> with a failure condition not satisfied." Awkward wording.

Thanks, I paraphrased the description.

English is not my native language, so I would appreciate suggestions to
improve the docs. Preferably, in the form of PRs.

> 23. "Additionally, the failure condition function object operator()
> must not throw," where is this enforced?

It is not enforced. I'm not enforcing it because this will effectively
prohibit using raw functions as condition functions prior to C++17,
which added noexcept to function types. I don't think it is worth
enforcing in C++17 onwards either, as this would introduce
incompatibility between different standard versions.

> 24. explicit scope_success(F&& func, C&& cond, bool active = true)
> can't you take the class-level template parameters by value, and rely
> on implicit conversion of the provided arguments ? That would reduce
> template instantiations substantially.

This would add an extra copy/move to initialize the scope guard members.

> 25. include/boost/scope/scope_success.hpp: I'm wary of the factory
> functions like make_scope_success. This seems like an anti-pattern
> that's only implemented because prior versions of C++ don't have the
> needed expressiveness, like deduction guides.

Right. Factory functions are there exactly to support C++11.

> 26. include/boost/scope/error_code_checker.hpp Why store the error
> code by pointer, and not by normal lvalue-reference (const, even) ?

Storing a pointer keeps the error_code_checker class assignable. Not
that it should matter for scope guards, but still might be useful in
some contexts.

> 27. A general comment about the implementation, i don't like how many
> places use std::enable_if. This is an immense compile-time slowdown.
> C++20 concepts appear to be much faster, but avoiding the
> metaprogramming where practical is much desired. Maybe static_assert()
> on various restrictions and simply don't provide fallbacks for things
> that aren't satisfied. For example, is it really important to support
> final classes? If not, you can cut your implementation by around
> 1/4th.

Concepts are C++20, so not acceptable for me.

static_asserts are not a replacement for SFINAE. SFINAE is used
specifically to avoid compilation errors and guide overload resolution
where needed. With constructors and assignment, SFINAE plays its role
with type traits that users (or even Boost.Scope code itself) may use to
limit its downstream overloads. For example, you can see how scope guard
factory functions test whether the respective scope guard can be
constructed from the arguments.

> 28. Table 1.1. Boost.ScopeExit and Boost.Scope syntax overview: why no
> macro for the failure handler?

As answered to another reviewer, I didn't see enough motivation to
provide macros for scope_exit/success/fail. Those scope guards are more
likely to be interacted with after construction, and the construction
itself is more elaborate, which doesn't work with syntax used by
BOOST_SCOPE_FINAL.

> 29. html/scope/scope_guards.html: "an lvalue reference to a function
> taking no arguments.", can't give you a function pointer? it has to be
> an lvalue-reference?

I've updated the docs (and code) on develop to work with function pointers.

> 30. html/scope/scope_guards.html: "For this reason, action functions
> that are used with scope_fail are typically not allowed to throw, as
> this may cause the program to terminate." you aren't wrong, but why is
> it boost.scope's job to enforce this?

It doesn't enforce it. The discussion you quoted is just a
recommendation to the user.

> 31. Table 1.3. Comparison of scope_fail and scope_exit: Your examples
> are confusing, because the vector would have already undone it's push
> if the push threw an exception.

There are two vectors. The scope guard reverts the push_back on the
first if the second one throws.

> 32. html/scope/unique_resource.html: IMHO I don't think
> unique_resource belongs in this library. It doesn't appear to re-use
> any of the code from the scope_*** family of objects, and seems more
> appropriate to put into either the Boost.SmartPtr library, make a new
> library for SmartResources, or SmartPtr, SmartResources, and Scope
> should all go into a "Boost.Raii" library.

Will answer separately.

Regarding the implementation, unique_resource does share some
implementation bits with scope guards. Not that there is very much to
share, but still.

> 33. for set_active(), personally i'd rather have a enable() and
> disable() (or whatever name) function so that I can take the address
> of the member function as a parameterless function, and do things with
> it that way. The need for that is infrequent, but it's served me well
> in the past.

As answered earlier, I strongly prefer a bool parameter to a pair of
functions, because you sometimes receive that bool (or something
similar) from elsewhere.

In your case, you wouldn't be taking address of a function (which is a
bad thing anyway, because the signature may change), but instead you
would be keeping that bool.

> 34. Should all scope guards be able to be activated / deactivated?
> You'd save a lot of stack space for users if they had the ability to
> opt-in to that ability instead of always having it.

There is scope_final that doesn't support cancellation.