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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__detail/transaction.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
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/`


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


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


================
Comment at: libcxx/include/__detail/transaction.h:52
+struct __transaction {
+    __transaction() = default;
+
----------------
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.


================
Comment at: libcxx/test/libcxx/detail/transaction.pass.cpp:125
+    return 0;
+}
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115730