[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:55
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit __transaction(_Rollback __rollback)
+        : __rollback_(_VSTD::move(__rollback))
----------------
ldionne wrote:
> 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.
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.


================
Comment at: libcxx/include/__detail/transaction.h:81-82
+private:
+    [[no_unique_address]] _Rollback __rollback_;
+    bool __completed_;
+};
----------------
ldionne wrote:
> 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`.
I think `optional` would also give EBO, but given the discussion above, this isn't important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115730