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/utilitie... 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.