EricWF added a comment. drive-by comments.
================ Comment at: include/experimental/functional:159 @@ +158,3 @@ + _LIBCPP_INLINE_VISIBILITY + void insert(const key_type &__key, value_type __val) + { ---------------- Do we want to copy the `__val` here? ================ Comment at: include/experimental/functional:161 @@ +160,3 @@ + { + __table [__key] = __val; // Would skip_.insert (val) be better here? + } ---------------- > Would skip_.insert(val) be better here? I don't think so but it depends on the semantics of this `insert` method. Should existing values be overwritten? ================ Comment at: include/experimental/functional:181 @@ +180,3 @@ + typedef typename std::make_unsigned<key_type>::type unsigned_key_type; + typedef std::array<_Value, 1U << (CHAR_BIT * sizeof(key_type))> skip_map; + skip_map __table; ---------------- `std::numeric_limits<unsigned_key_type>::max()` might be clearer. It took me a while to figure out what this was doing. ================ Comment at: include/experimental/functional:192 @@ +191,3 @@ + _LIBCPP_INLINE_VISIBILITY + void insert(key_type __key, value_type __val) + { ---------------- Do we want the extra copy of `__val` here? ================ Comment at: include/experimental/functional:215 @@ +214,3 @@ + _VSTD::is_integral<value_type>::value && // what about enums? + sizeof(value_type) == 1 && + is_same<_Hash, hash<value_type>>::value && ---------------- Is this only meant to trigger for `char` and `bool`? I think it would be wrong to use the array specialization for `bool` because it can only represent 2 values but the array will end up having a size of `256`. http://reviews.llvm.org/D11380 _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits