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

From: Дмитрий Архипов (grisumbras_at_[hidden])
Date: 2023-11-30 09:29:14

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/utilities.html
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_at_[hidden]>:
>
> 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/scope.hpp).
>
> 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.