[libcxx-commits] [PATCH] D115730: [libc++] Add a helper class to write code with the strong exception guarantee
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Dec 14 15:02:28 PST 2021
ldionne added inline comments.
================
Comment at: libcxx/include/CMakeLists.txt:143
__debug
+ __detail/transaction.h
__errc
----------------
Mordante wrote:
> Just curious what is the `__detail` directory intended to contain?
It was meant to contain implementation details of the library that don't really fit in something else like `__memory/`, `__algorithm/` or something like that. But per Arthur's comment, I will delay having that discussion to later.
================
Comment at: libcxx/include/__detail/transaction.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
Quuxplusone wrote:
> I suggest that this should go in `__utility/` for now — alongside `priority_tag` and (D115686 `auto_cast`) and `decay_copy` and `to_underlying` and so on.
> I can imagine a direction where we take all those "truly internal internals" and move them all into `__detail/` or `__misc/` or something, to indicate that they're intentionally not directly included by any //standard// header... but I don't see a compelling reason for this //one// facility to be inconsistent with those others for now.
>
> Also, putting it in `<utility>` sidesteps issues such as
> - Can we have a detail header without a top-level //module//?
> - If this detail header is its own top-level module, that breaks the invariant that detail headers are never included directly (see: your new test)
> - Bikeshedding the name of `__detail/`
This is not the first time it happens :-). I had originally put it in `__utility/` and then I moved it to `__detail` because I found it weird to include it in `<utility>`. But yeah, I agree with your pragmatic approach. I do think we will want a true directory for internal implementation details, but it's easy to create that later.
================
Comment at: libcxx/include/__detail/transaction.h:20
+
+#if _LIBCPP_STD_VER > 17
+
----------------
var-const wrote:
> Quuxplusone wrote:
> > If this facility is useful, it might be useful in C++11/14 as well. I think it's worth thinking now about how it could be enabled for C++11.
> It seems like this would significantly limit the scope where `__transaction` can be used internally. From a quick glance, it seems like `[[no_unique_address]]` can be replaced by EBO and the rest of the class should pretty much work for all language modes that support `constexpr`, with minor adjustments.
Yup, agreed, will adjust it. I've got the tests working in C++11 and above. Porting them to C++03 is just too painful cause we can't use lambdas at all, so I'll make an effort to make `__transaction` work in C++03, but I won't port the test. LMK if you're not OK with that.
Regarding `[[no_unique_address]]`, I actually suggest removing it altogether if we want to support this in prior standards. Concretely, we will almost always capture some state in the lambda to perform the rollback, so there would be no optimization here. Furthermore, this isn't the kind of type that we will store and have many instances of, and so IMO optimizing the storage is not as important as e.g. `std::string` or `std::optional`.
================
Comment at: libcxx/include/__detail/transaction.h:31
+//
+// Transactions can't be copied or assigned to, but they can be moved around for convenience.
+//
----------------
var-const wrote:
> Quuxplusone wrote:
> > I suggest eliminating the move-constructor/assignment, since I can't think of any place it would be useful. Passing a transaction up or down the call stack seems like an antipattern: it's designed to replace `try/catch`, which is //lexically//, not dynamically, scoped. I'd make it non-movable, like `mutex`.
> I would also add that adding move support later on is a straightforward extension, unlike the other way round.
I don't have an extremely strong preference to keep move construction (move assignment is already disabled), however IMO we'd be artificially limiting the convenience of the class for little reason. For now I've added tests for `noexcept`-ness and so on, but let me know if you feel strongly that the class would be better without being move constructible and I can change that. My rationale for keeping it would go along the lines of "It's implemented, tested and it matches what `experimental::scope_exit` does" -- but I don't feel strongly about it.
================
Comment at: libcxx/include/__detail/transaction.h:48
+// return out;
+// }
+//
----------------
Mordante wrote:
> I like this comment a lot!
Thank you!
================
Comment at: libcxx/include/__detail/transaction.h:51
+template <class _Rollback>
+struct __transaction {
+ __transaction() = default;
----------------
Mordante wrote:
> Somewhat curious, did you consider implementing `scope_exit` instead of `__transaction`?
> https://en.cppreference.com/w/cpp/experimental/scope_exit/scope_exit
>
> (I know that isn't `constexpr` (yet))
I had not. However, since `__transaction` is for internal use and we can't use experimental APIs internally (cause those go away eventually), I think it makes sense to keep those separate.
================
Comment at: libcxx/include/__detail/transaction.h:52
+struct __transaction {
+ __transaction() = default;
+
----------------
Quuxplusone wrote:
> This default-constructs the `_Rollback` and leaves random garbage in `__completed_`, so this is bad.
> I'm confident that this class should //not// be default-constructible.
My thinking was that it might be useful in case we wanted to define rollback instructions in a stateless function object, not as a lambda. However, since we will usually need to capture some state in the function we're guarding, default construction should rarely (if ever) be useful. Will remove it for now, and we can add it later if there is a use case.
================
Comment at: libcxx/include/__detail/transaction.h:55
+ _LIBCPP_HIDE_FROM_ABI
+ constexpr explicit __transaction(_Rollback __rollback)
+ : __rollback_(_VSTD::move(__rollback))
----------------
var-const wrote:
> Question: would it make sense to create both `(_Rollback&&)` and `(const _Rollback&)` to avoid an extra move?
I thought about it, and decided not to do it because this should generally be initialized by a cheap-to-move (and even cheap-to-copy) function object. I don't think it will ever matter, but if you think it may, let me know and I'll turn this into two overloads.
================
Comment at: libcxx/include/__detail/transaction.h:75
+ _LIBCPP_HIDE_FROM_ABI
+ _LIBCPP_CONSTEXPR_AFTER_CXX17 ~__transaction() {
+ if (!__completed_)
----------------
var-const wrote:
> Mordante wrote:
> >
> Nit: because this class only works in C++20 and above, it seems like this could be just `constexpr`. However, if you plan to make it available for earlier language versions, then it's not worth spending the time changing this.
Yeah, I was kind of on the fence when I wrote it, but I'll make it work in all standard modes, so this is going to stay.
================
Comment at: libcxx/include/__detail/transaction.h:81-82
+private:
+ [[no_unique_address]] _Rollback __rollback_;
+ bool __completed_;
+};
----------------
var-const wrote:
> Hmm, it kinda looks like an `optional<_Rollback>`, but going that route would make it hard to have this class available in older language modes.
Also, IMO the class is so tiny it makes sense to just keep the `bool` there -- we wouldn't be simplifying much by using `optional`.
================
Comment at: libcxx/test/libcxx/detail/transaction.pass.cpp:65
+
+ // Basic properties of the type
+ {
----------------
var-const wrote:
> Question: would it make sense to also check `sizeof` to make sure the `[[no_unique_address]]` optimization works as expected?
See my comment above, but I suggest removing `[[no_unique_address]]` altogether if we support older standard modes. Otherwise, yes I think it would have made sense.
LMK if you disagree with the decision to remove `[[no_unique_address]]`.
================
Comment at: libcxx/test/libcxx/detail/transaction.pass.cpp:125
+ return 0;
+}
----------------
Quuxplusone wrote:
> I'd also like to see that it works properly when called from a destructor. (This is more obviously a big deal for my alternative `__on_exception_rollback` design, but might as well check it no matter which design we pick.)
> ```
> bool rolled_back = false;
> struct S {
> ~S() {
> std::__transaction t([&]{ rolled_back = true; });
> t.__set_completed();
> }
> };
> try {
> S s;
> throw 0;
> } catch (...) {
> assert(!rolled_back);
> }
> ```
> (I'd say if `S` has a //data member// of type `__transaction`, that produces weird behavior but also violates the intended usage of the type, and so we shouldn't worry about //that// case.)
Adding the test.
Sorry, what does `__on_exception_rollback` do? Does it check whether the destructor is being called during stack unwinding?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115730/new/
https://reviews.llvm.org/D115730
More information about the libcxx-commits
mailing list