[PATCH] D11380: Implement LFTS searchers. Boyer_Moore and Boyer_Moore_Horspool

[Eric Fiselier]

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