__transaction is a helper class that allows rolling back code in case an
exception is thrown. The main goal is to reduce the clutter when code
needs to be guarded with #if _LIBCPP_NO_EXCEPTIONS.
Details
- Reviewers
Mordante - Group Reviewers
Restricted Project - Commits
- rG37e6bd8bc8da: [libc++] Add a helper class to write code with the strong exception guarantee
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__detail/transaction.h | ||
---|---|---|
1 ↗ | (On Diff #394248) | 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. Also, putting it in <utility> sidesteps issues such as
|
20 ↗ | (On Diff #394248) | 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. |
31 ↗ | (On Diff #394248) | 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. |
52 ↗ | (On Diff #394248) | This default-constructs the _Rollback and leaves random garbage in __completed_, so this is bad. |
libcxx/test/libcxx/detail/transaction.pass.cpp | ||
125 ↗ | (On Diff #394248) | 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.) |
In general LGTM, some small nits and questions.
libcxx/include/CMakeLists.txt | ||
---|---|---|
143 | Just curious what is the __detail directory intended to contain? | |
libcxx/include/__detail/transaction.h | ||
48 ↗ | (On Diff #394248) | I like this comment a lot! |
51 ↗ | (On Diff #394248) | Somewhat curious, did you consider implementing scope_exit instead of __transaction? (I know that isn't constexpr (yet)) |
60 ↗ | (On Diff #394248) | |
60 ↗ | (On Diff #394248) | Should this function be conditionally noexcept? |
75 ↗ | (On Diff #394248) |
libcxx/include/__detail/transaction.h | ||
---|---|---|
20 ↗ | (On Diff #394248) | 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. |
31 ↗ | (On Diff #394248) | I would also add that adding move support later on is a straightforward extension, unlike the other way round. |
55 ↗ | (On Diff #394248) | Question: would it make sense to create both (_Rollback&&) and (const _Rollback&) to avoid an extra move? |
75 ↗ | (On Diff #394248) | 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. |
81–82 ↗ | (On Diff #394248) | 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. |
libcxx/test/libcxx/detail/transaction.pass.cpp | ||
65 ↗ | (On Diff #394248) | Question: would it make sense to also check sizeof to make sure the [[no_unique_address]] optimization works as expected? |
Address review comments.
libcxx/include/CMakeLists.txt | ||
---|---|---|
143 | 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. | |
libcxx/include/__detail/transaction.h | ||
1 ↗ | (On Diff #394248) | 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. |
20 ↗ | (On Diff #394248) | 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. |
31 ↗ | (On Diff #394248) | 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. |
48 ↗ | (On Diff #394248) | Thank you! |
51 ↗ | (On Diff #394248) | 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. |
52 ↗ | (On Diff #394248) | 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. |
55 ↗ | (On Diff #394248) | 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. |
75 ↗ | (On Diff #394248) | 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. |
81–82 ↗ | (On Diff #394248) | 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. |
libcxx/test/libcxx/detail/transaction.pass.cpp | ||
65 ↗ | (On Diff #394248) | 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]]. |
125 ↗ | (On Diff #394248) | Adding the test. Sorry, what does __on_exception_rollback do? Does it check whether the destructor is being called during stack unwinding? |
libcxx/include/__detail/transaction.h | ||
---|---|---|
55 ↗ | (On Diff #394248) | I think I have a very slight preference towards doing two overloads. My reasoning is that this helper class should ideally add no performance penalty (and minimal performance penalty in non-optimized builds), and it being low/no-overhead makes it more attractive for internal usage. A combinatorial explosion of overloads seems very unlikely, and two overloads I think are tolerable. Then again, I might be overly cautious about performance issues here. I don't feel very strongly about this and can certainly be convinced that this isn't important in this case. |
81–82 ↗ | (On Diff #394248) | I think optional would also give EBO, but given the discussion above, this isn't important. |
libcxx/include/__utility/transaction.h | ||
---|---|---|
80 | Would it be possible to get rid of __complete() by replacing this with something like if (std::uncaught_exceptions() > initial_exception_count) and initializing initial_exception_count = std::uncaught_exceptions() in the constructor? |
libcxx/test/libcxx/detail/transaction.pass.cpp | ||
---|---|---|
125 ↗ | (On Diff #394248) | Yeah, exactly what @DanielMcIntosh-IBM just said elsewhere in this review. :) However, taking another page from the Auto.h playbook, I think you should consider eliminating the completed_ member altogether and just making the lambda always run on destruction. If the programmer wants the lambda's behavior to be conditional, they can just... put an if in it. bool completed = false; __on_scope_exit guard([&]() { if (!completed) puts("Rolling back!"); }); puts("Starting transaction"); f(); completed = true; // guard runs here IMO, this is a more readable API than having a magic .set_completed() method on the guard object itself. |
libcxx/include/__utility/transaction.h | ||
---|---|---|
52 | Why not omit this? | |
55 | I see a lot of different constexpr macros in this class | |
62 | The tests require C++11 or later. | |
libcxx/test/libcxx/utilities/transaction.pass.cpp | ||
155 | Please make sure this version matches the version in which the class is intended to be constexpr. |
libcxx/include/__utility/transaction.h | ||
---|---|---|
55 | This is presumably due to a style guideline that I made up ;) a while ago: "use the most aggressive constexpr macro possible for each place in internal code," so that if we ever drop support for C++03 (or Clang gains support for constexpr as an extension in C++03 mode), we can replace the absolute maximum number of uses of _LIBCPP_CONSTEXPR in one fell swoop. I.e. I think this is fine. But, given the intended usage of this class... do we need it to be constexpr at all? Certainly not for call_once or uninitialized_copy_n. @ldionne, could you add some sample usages (in places like <algorithm> and the various containers, maybe) that show why constexprness is a good idea? Or if no great examples are conspicuous, I'd recommend removing the constexpr keywords entirely for now. |
libcxx/include/__utility/transaction.h | ||
---|---|---|
55 | I wasn't aware of this new style and haven't noticed it before during reviews. To me it feels odd especially when the destructor is the "limiting factor". To be honest I personally don't like this style. That being said, if this is the style we've used before, then I withdraw my earlier comment about mixing the macros. |
libcxx/include/__utility/transaction.h | ||
---|---|---|
55 | The rule makes a lot more sense (IMO) when it's talking about some random free function, like e.g. __sift_up or __sort3 (constexpr-after-11, even though their only callers are constexpr-after-17). I'll gladly agree that a class with a bunch of member functions at different levels is a pretty pathological case for this rule (although I'll be at-best-ambivalent as to whether that means we should abandon the rule even in this pathological case). |
Fix constexpr macros
libcxx/include/__utility/transaction.h | ||
---|---|---|
52 | I like calling out explicitly the properties of types instead of relying too much on the implicit rules, because these rules are pretty complicated. For example, see how I explicitly delete the copy constructor and the assignment operators below -- I could have relied on any number of implicit rules (e.g. the fact that we define a destructor explicitly) to get the same effect, but IMO it forces the reader to be more cleaver than I want. LMK if you are not convinced and I'll remove it. | |
55 | My original intent was to make things constexpr as early as possible, but I agree this ends up being somewhat messy. Instead, I've made everything be constexpr in C++20 and above only. The benefit of making this class constexpr is that it can be used inside functions that are themselves constexpr (there's more than just the uninitialized_foo family that will benefit from this). If we want to use it in a C++11/C++14/C++17 constexpr function, we can come back and try to make it work. @Quuxplusone An example would be std::vector::insert. Like I said, we can come back later and make it constexpr friendly in older standards if we have a use for it, but I wouldn't de-constexprify this upfront. | |
62 | The tests do, however we're still going to use the class in places that are supported in C++03, like some algorithms, so I can't use raw noexcept. | |
80 | Yes, however as Arthur explained in another comment (https://reviews.llvm.org/D115730?id=394248#inline-1106466), we don't want to add a call to uncaught_exceptions() on the success path. The benefit of the current approach is that the compiler will be able to see that we call __complete() on all happy paths, and as a result it should be able to do optimal codegen. |
LGTM!
libcxx/include/__utility/transaction.h | ||
---|---|---|
52 | Actually I tend to do the same for copy/move constructor/assignment for the same reasons. So let's keep it. |
I'll land this -- please leave any comments here and I can apply them post-commit if needed.
@ldionne: I'd still prefer the version I showed in https://reviews.llvm.org/D115730?id=394248#inline-1106466 where the object doesn't have a completed data member at all (so it would better be named __on_scope_exit rather than __transaction). I'd also be interested in seeing a godbolt or something showing which approach gives better codegen. Naïvely, I would expect my preferred version would be no worse, and maybe a tiny bit better, because it gives the optimizer more freedom to rearrange the stack frame. But the reason I prefer it is for simplicity/readability: I think the set_completed/__complete thing is easy to mess up.
I have a strong distaste for managing the bool completed and having to enclose all the rollback code in if (!completed). IMO that's a lot easier to mess up than just making sure a __complete() method is called. Since this simple helper is really tailored for exception handling (it's purely a means to remove #if _LIBCPP_NO_EXCEPTIONS), I'd rather keep as-is.
Just curious what is the __detail directory intended to contain?