On Sun, Jan 24, 2016 at 9:46 PM, Duncan P. N. Exon Smith < dexonsm...@apple.com> wrote:
> Great, I should have time to clean this up tomorrow afternoon. > > Regarding emplace, this patch as is has tests for emplace, but > they depend on r258575 being in tree. You asked me to revert > that... I'll wait for your response over in that thread: > > http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147795.html > > I have a few other questions inline below. > > > On 2016-Jan-24, at 19:25, Eric Fiselier <e...@efcs.ca> wrote: > > > > > > ================ > > Comment at: include/unordered_map:401 > > @@ -400,1 +400,3 @@ > > {return static_cast<const _Hash&>(*this)(__x.__cc.first);} > > +#if defined(_LIBCPP_HAS_NO_RVALUE_REFERENCES) > > + _LIBCPP_INLINE_VISIBILITY > > ---------------- > > Is there a reason your hiding these overloads in C++11 and beyond? > > > > I would prefer as few conditional compilation blocks as possible. > > In C++11, this code is dead and untested. Happy to drop the > conditionals though, it's tested in C++03 so it should be fine. > It's actually not dead in C++11, its just not tested. Without this overload the hasher will still *accept* "value_type" but it performs an implicit conversion to __hash_value_type. This is super bad. I think __hash_value_type's ctors should be made explicit but I'm not sure. > On second thought, I wonder if it would be possible to skip > this, and use the key directly like in C++11? I'll take a > look. > > > ================ > > Comment at: include/unordered_map:1004 > > @@ +1003,3 @@ > > + _LIBCPP_INLINE_VISIBILITY > > + pair<iterator, bool> __insert_extract_key_if_pair(pair<_FirstTp, > _SecondTp>&& __x) > > + {return __insert_extract_key<_FirstTp>(_VSTD::move(__x));} > > ---------------- > > Overall the dispatch looks good, but it could be a lot simpler. Instead > of multi-level dispatch I would write a single trait to computer if "_Pp" > is a keylike pair. Then `insert(_Pp&&)` just use that to call > `__insert_extract_key_if_referenceable` directly using that trait. > > > > For example: > > ``` > > template <class _Key, class _Pair, class _RawPair = typename > __uncvref<_RawPair>::type> > > struct __is_keylike_pair : false_type {}; > > > > template <class _Key, class _Pair, class _First, class _Second> > > struct __is_keylike_pair<_Key, _Pair, pair<_First, _Second> > : > is_same<typename remove_const<_First>::type, _Key> {}; > > ``` > > Above the unordered_map<> definition a good place for this, or > would it be better to drop it in type_traits or some such? > Somewhere in <unordered_map> is fine. > > > We also should handle `pair<...>&` which you previously didn't have. > > I guess that got lost when I switched away from the lazy logic > using decay. I'll add it back. Strange that none of the tests > failed... I guess I'll need to find a way to cover that case.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
