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. 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)? I recommend acceptance assuming the above points are addressed. #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. 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. 3. #ifdef BOOST_HAS_PRAGMA_ONCE : why bother? you have a normal include guard. What does this do for you? The major compilers use the same code paths for pragma once as they do for include guards, or so i read on some blog that seemed authoratitve a long time ago. 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. 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. 6. include/boost/scope/detail/is_not_like.hpp: no human readable explanation of what this type-trait is supposed to do. 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 ? 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]]. 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. 10. include/boost/scope/detail/compact_storage.hpp: Instead of get() member functions, what about implicit conversion operators instead? 11. include/boost/scope/unique_resource.hpp: Can't you use boost::is_nothrow_invocable<> for the ref_wrapper::operator() noexcept specification? 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. 13. include/boost/scope/unique_resource.hpp : resource_holder::get() functions. Why not use using resource_base::get; instead? More compact. 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? 15. unique_resource_data::swap_impl: std::swap(m_allocated, that.allocated); 16. static_cast< unique_resource_data&& >(that) : std::move() by any other name? 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. 18. unique_resource.hpp: "The deleter must be copy-constructible." Perhaps discuss whether move-constructible matters at all. 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? 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? 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? 22. "Scope exit guard that invokes a function upon leaving the scope with a failure condition not satisfied." Awkward wording. 23. "Additionally, the failure condition function object operator() must not throw," where is this enforced? 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. 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. 26. include/boost/scope/error_code_checker.hpp Why store the error code by pointer, and not by normal lvalue-reference (const, even) ? 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. 28. Table 1.1. Boost.ScopeExit and Boost.Scope syntax overview: why no macro for the failure handler? 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? 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? 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. 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. 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. 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. After some persuasion, the second redditor has added a resolution, turning his code review into a proper review: Conditional reject: * This library should not have multiple separate tools that don't share a common theme. The resource management functionality is not appropriate to mix into the same namespace as generic scope guard stuff, and is more appropriately added to the SmartPtr library, or a new SmartResources library. * I strongly dislike the default-disablement of compiler warnings. I have SO MANY patches on top of boost (which any time I've submitted them as PRs to github have been rejected, so i stopped bothering years ago) to squash all the warnings I get from boost, and I don't want more of that. If your code isn't warning free without suppression pragmas, it shouldn't be submitted to boost, with the only exception being warnings that the compiler is producing erroneously, and the suppression for those should be as narrowly scoped as possible with justification put as a comment along side the pragma about why the compiler can't be satisfied without the pragma * Too many C++03-isms: Targeting C++11 does a major disservice to the world. C++11 was released over a decade ago. Move forward. Target C++14 at a minimum, but C++17 is ideal. I won't say to target C++20, since that's not practical for most organizations at this time (given compilers only recently, e.g. last 6 months, had viable implementations of C++20 that didn't internal-compiler-error all over the place). I won't say that targeting C++14/17 is a part of the conditional rejection, but if you're going to target C++11, then use C++11 functionality, like using. * Where ever possible (e.g. supported by your targeted language version), use the _t and _v version of type-traits over the thing<>::value and thing<>::type version, otherwise you're forcing your consumer's compilers to do notably more work. That's a narrowly scoped list of 4 action items for my opinion to change from reject to accept. пт, 24 нояб. 2023 г. в 23:58, Дмитрий Архипов <grisumbras@gmail.com>:
Dear Boost community. The peer review of the proposed Boost.Scope will start on 26th of November and continue until December 5th. Boost.Scope is a small library implementing utilities defined in <experimental/scope> from C++ Extensions for Library Fundamentals v3 with a few extensions. Namely, the library contains: * A number of scope guards for various use cases. * A unique resource wrapper that automatically frees the resource on destruction. * Utilities for wrapping POSIX-like file descriptors in the unique resource wrapper.
You can find the source code of the library at https://github.com/Lastique/scope and read the documentation at https://lastique.github.io/scope/libs/scope/doc/html/index.html. The library is header-only and thus it is fairly easy to try it out. In addition, the library can be used with Conan and vcpkg (see its README for instructions on how to do that). Finally, there's a single header version suitable for Compiler Explorer (https://raw.githubusercontent.com/Lastique/scope/single-header/include/boost...).
As the library is not domain-specific, everyone is very welcome to contribute a review either by sending it to the Boost mailing list, or me personally. In your review please state whether you recommend to reject or accept the library into Boost, and whether you suggest any conditions for acceptance.
Thanks in advance for your time and effort!
Dmitry Arkhipov, Staff Engineer at The C++ Alliance.