[libcxx-commits] [PATCH] D115730: [libc++] Add a helper class to write code with the strong exception guarantee

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__detail/transaction.h:20
+
+#if _LIBCPP_STD_VER > 17
+
----------------
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.


================
Comment at: libcxx/include/__detail/transaction.h:31
+//
+// Transactions can't be copied or assigned to, but they can be moved around for convenience.
+//
----------------
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.


================
Comment at: libcxx/include/__detail/transaction.h:55
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit __transaction(_Rollback __rollback)
+        : __rollback_(_VSTD::move(__rollback))
----------------
Question: would it make sense to create both `(_Rollback&&)` and `(const _Rollback&)` to avoid an extra move?


================
Comment at: libcxx/include/__detail/transaction.h:75
+    _LIBCPP_HIDE_FROM_ABI
+    _LIBCPP_CONSTEXPR_AFTER_CXX17 ~__transaction() {
+        if (!__completed_)
----------------
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.


================
Comment at: libcxx/include/__detail/transaction.h:81-82
+private:
+    [[no_unique_address]] _Rollback __rollback_;
+    bool __completed_;
+};
----------------
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.


================
Comment at: libcxx/test/libcxx/detail/transaction.pass.cpp:65
+
+    // Basic properties of the type
+    {
----------------
Question: would it make sense to also check `sizeof` to make sure the `[[no_unique_address]]` optimization works as expected?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115730/new/

https://reviews.llvm.org/D115730