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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a subscriber: EricWF.
Mordante added inline comments.


================
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));
----------------
ldionne wrote:
> Mordante wrote:
> > 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.)
> > 
> > 
> > 
> I fully agree that we shouldn't blindly use `auto`, I think it generally obscures the code. IMO these "almost always use auto" advice should never have been given, they were just too easy to mis-interpret and exaggerate on.
> 
> That being said, the alternative here is:
> 
> ```
> std::__transaction<_AllocatorDestroyRangeReverse<_Alloc, _Iter2> > __guard(_AllocatorDestroyRangeReverse<_Alloc, _Iter2>(__alloc, __destruct_first, __first2));
> ```
> 
> IMO, since we have a `std::__make_transaction` function, we don't gain much from repeating the type. This is just my .02, I don't really care about this specific instance. I think this one is reasonable, but I agree with @Mordante in the general case about not abusing `auto` (I personally don't remember seeing @philnik abuse it, though).
I've seen several places where `auto` was used which resulted in me to spending extra time to figure out the exact type used. I've seen @EricWF asking about less `auto` in the past too. (I don't have concrete examples, and I don't really want to spend time looking at when I asked it.)

Since `__make_transaction` is not a standard function I need to check whether it really creates a `__transation`. Using `__transaction` instead of `auto` would have saved me that verification.

Another issue I have with `auto` that it's not always clear what the intended type is. Especially when relying on C++ rules; where I'm not sure whether the writer had the same view of these rules as I have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133661