[PATCH] D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets

[Louis Dionne via Phabricator via cfe-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Generally looks pretty good, but I have a couple questions/suggestions.



================
Comment at: libcxx/include/__hash_table:2165
+#if _LIBCPP_STD_VER > 14
+template <class _Tp, class _Hash, class _Equal, class _Alloc>
+template <class _NodeHandle, class _InsertReturnType>
----------------
When a function is declared with a visibility macro (`_LIBCPP_INLINE_VISIBILITY` in this case), I think it is customary in libc++ to repeat the visibility macro on the definition as well. If that's correct, apply here and to other similar functions below, and also in `__tree`.


================
Comment at: libcxx/include/__hash_table:2212
+    allocator_type __alloc(__node_alloc());
+    return _NodeHandle(remove(__p).release(), __alloc);
+}
----------------
Just a quick Q: am I mistaken or `remove()` is not part of the `unordered_map` API? We're not naming it `__remove()` just because we already have other functions called `remove()`, so any macro redefining `remove` would already break things -- is that the idea?


================
Comment at: libcxx/include/__node_handle:33
+template <class _NodeType, class _Alloc>
+struct __generic_container_node_destructor;
+
----------------
Clever way of reducing header dependencies. I like it.


================
Comment at: libcxx/include/__node_handle:36
+template <class _NodeType, class _Alloc>
+class __node_handle_base
+{
----------------
I'd like to suggest a different approach for implementing the node handles for sets and maps. I believe this would simplify things slightly:

```
template <class _NodeType, class _Alloc, template <class...> class _MapOrSetSpecifics>
class __basic_node_handle 
    : public _MapOrSetSpecifics<_NodeType, __basic_node_handle<_NodeType, _Alloc, _MapOrSetSpecifics>>
{
    // same as right now
};
```

Then, you can get the two different node handles by simply providing the methods in the base class:

```
template <class _NodeType, class _Derived>
struct __map_node_handle_specifics {
    using key_type = typename _NodeType::__node_value_type::key_type;
    using mapped_type = typename _NodeType::__node_value_type::mapped_type;

    _LIBCPP_INLINE_VISIBILITY
    key_type& key() const
    {
        return static_cast<_Derived const*>(this)->__ptr_->__value_.__ref().first;
    }

    _LIBCPP_INLINE_VISIBILITY
    mapped_type& mapped() const
    {
        return static_cast<_Derived const*>(this)->__ptr_->__value_.__ref().second;
    }
};

template <class _NodeType, class _Derived>
struct __set_node_handle_specifics {
    using value_type = typename _NodeType::__node_value_type;

    _LIBCPP_INLINE_VISIBILITY
    value_type& value() const
    {
        return static_cast<_Derived const*>(this)->__ptr_->__value_;
    }
};

template <class _NodeType, class _Alloc>
using __map_node_handle = __basic_node_handle<_NodeType, _Alloc, __map_node_handle_specifics>;

template <class _NodeType, class _Alloc>
using __set_node_handle = __basic_node_handle<_NodeType, _Alloc, __set_node_handle_specifics>;
```

This is a classic application of the CRTP. I believe it would reduce the amount of boilerplate currently required for special member functions. It's possible that you thought of this and realized it didn't work for a reason I'm missing right now, too, but the idea seems to work on the surface: https://wandbox.org/permlink/nMaKEg43PVJpA0ld.

You don't have to do this, but please consider it if you think it would simplify the code.



================
Comment at: libcxx/include/map:915
+
+    __base& __get_tree() { return __tree_; }
+
----------------
I believe this function should be marked with `_LIBCPP_INLINE_VISIBILITY` -- even though in practice the compiler will probably always inline it.

Also, you don't seem to be using that function at all in the diff -- is that right?

So please either mark with `_LIBCPP_INLINE_VISIBILITY` and use it, or remove it altogether. This comment applies to all 8 containers to which similar methods were added.


================
Comment at: libcxx/include/set:399
 
+template <class _Key, class _Compare, class _Allocator>
+class multiset;
----------------
Is this forward declaration necessary?


================
Comment at: libcxx/include/unordered_set:569
+            "node_type with incompatible allocator passed to unordered_set::insert()");
+        return insert(_VSTD::move(__nh)).position;
+    }
----------------
Why don't you use
```
return __table.template __node_handle_insert_unique<node_type>(__hint, _VSTD::move(__nh));
```

instead? It seems like this way, if `__node_handle_insert_unique(const_iterator, node_type)` eventually starts taking advantage of the iterator hint, you'd get the improvement here for free.


================
Comment at: libcxx/include/unordered_set:1069
+    _LIBCPP_INLINE_VISIBILITY
+    node_type extract(const_iterator __position)
+    {
----------------
For the other containers, the two `insert` methods come before the two `extract` methods. This is very nitpicky, but you should consider keeping the same order here too for consistency.


================
Comment at: libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_extract.pass.cpp:14
+
+// class unordered_map
+
----------------
Please be more specific about which functions you're testing. I find it useful to look at those comments to quickly see what exact signatures are being tested when I'm looking for tests that exercice some specific piece of code.

Actually, perhaps you should extract those tests into multiple files:
`unord.map/unord.map.modifiers/insert.node_handle.pass.cpp` => test overload of `insert` on `node_type`
`unord.map/unord.map.modifiers/insert.hint_node_handle.pass.cpp` => test overload of `insert` with a hint and a `node_type`
`unord.map/unord.map.modifiers/extract.node_handle.pass.cpp` => test overload of `extract` on `node_type`
`unord.map/unord.map.modifiers/extract.iterator.pass.cpp` => test overload of `extract` on iterator
`unord.map/insert_return_type.pass.cpp` => test `insert_return_type`
`unord.map/node_type.pass.cpp` => test node handle operations

I believe this would be more consistent with the way things are currently organized for other functions. This comment applies to all containers touched by this review.


================
Comment at: libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_extract.pass.cpp:23
+{
+typedef std::unordered_map<int, int> int_map_type;
+using intIRT = int_map_type::insert_return_type;
----------------
Consider using `using XXX = YYY` consistently.


================
Comment at: libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_extract.pass.cpp:33
+    std::equal_to<Counter<int> >,
+    min_allocator<std::pair<const Counter<int>, Counter<int> > > >
+    map_type;
----------------
You don't need spaces between consecutive `>`'s since those tests are not supported in C++03. Feel free to keep them or remove them, I'm just pointing it out.


================
Comment at: libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_extract.pass.cpp:99-101
+    the_map::iterator i = s.begin();
+    std::advance(i, 2);
+    assert(i == s.end());
----------------
Why is this test useful?


https://reviews.llvm.org/D46845