[libcxx-commits] [PATCH] D97862: [libc++] Introduce __identity_t<T>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/map:908
+    typedef __identity_t<_Compare>                   key_compare;
+    typedef __identity_t<_Allocator>                 allocator_type;
     typedef value_type&                              reference;
----------------
Quuxplusone wrote:
> @ldionne wrote:
> > This is non-blocking, however I think it might be nicer to use `__identity_t` on the arguments of the constructors that need them instead of on the typedefs. Otherwise, it's really unclear //why// this convolution is needed.
> 
> I agree, but when I started trying to do that, I immediately ran into existing implementation divergence; see https://godbolt.org/z/bxhn84 (libstdc++ rejects, libc++ and MSVC accept) and my tonight's LWG reflector thread "P1518 redux: CTAD deducing Allocator/Compare from random constructors". I definitely think the libc++/MSVC behavior is the "correct" behavior, so I'd want to preserve it, even if it means putting `__identity_t` on more ctor arguments than the paper standard currently mandates.
> 
> But now that's a feature-ish enough change that I'm gonna just land this "NFCI" commit as-is, and open a different PR if I decide it's worth doing anything else (which I still might, but it's no longer a slam-dunk).
Okay, I agree with that. Thanks for keeping changes small and separated!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97862