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

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

Quuxplusone 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;
----------------
@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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97862