[libcxx-commits] [PATCH] D133661: [libc++] Improve binary size when using __transaction

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

I'm not fond of this approach, due to the different behaviour whether or not exceptions are enabled.



================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:536
   auto __destruct_first = __first2;
-  try {
-#endif
+  auto __guard =
+      std::__make_transaction(_AllocatorDestroyRangeReverse<_Alloc, _Iter2>(__alloc, __destruct_first, __first2));
----------------
Can you please stop using auto everywhere? This has been requested several time by several reviewers. It really makes it unneeded hard to review code. The LLVM developer policy explains why this style isn't wanted
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable.

(In library code there might be reasons to use `auto` a bit more often, but this isn't one of these places.)





================
Comment at: libcxx/include/__utility/transaction.h:90-104
+#else
+template <class _Rollback>
+struct __transaction {
+  __transaction() = delete;
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __transaction(_Rollback) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __transaction(__transaction&&)
+      _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value) {}
----------------
huixie90 wrote:
> having this `__transaction` class as a noop class in `_LIBCPP_NO_EXCEPTIONS` mode might cause confusions, because the class name does not indicate anything to do with exceptions (and could be misused as a general purpose `scope_guard`).
> 
> what about having a function `__make_exception_guard`, which returns the `__transaction` class in the exception mode, and returns a `__noop_transaction` class in the noexception mode?
+1 for the confusion. What makes it more confusing is the usage of `_NOEXCEPT` in this class. Isn't that useless when exceptions are disabled? Please also some comment why this class it to be used as a no-op.

I'm not even sure whether we really want this class. I expect a transaction that is not committed when it goes out of scope to do a rollback.
```
void __foo() {
  __transaction __t;
  ... // do stuff
  if(!__happy())
    return;
  __t.__complete();
}

```
In that way this class behaves different depending on whether or not exceptions are enabled.

Based on the failure in the 'No exception' CI the behaviour above was intended to be working.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133661