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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
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>
----------------
ldionne wrote:
> 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`.
I can't say I know libc++'s policy here either, but it couldn't hurt, so I added the attributes to the new patch. Maybe @EricWF or @mclow.lists could weigh in here?


================
Comment at: libcxx/include/__hash_table:2212
+    allocator_type __alloc(__node_alloc());
+    return _NodeHandle(remove(__p).release(), __alloc);
+}
----------------
ldionne wrote:
> 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?
Yep, I think that was the thinking. IMO it would be nice to just call it `__remove` to avoid any confusion.


================
Comment at: libcxx/include/__node_handle:36
+template <class _NodeType, class _Alloc>
+class __node_handle_base
+{
----------------
ldionne wrote:
> 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.
> 
Oh, neat! That's a great idea, I didn't even consider CRTP here for some reason. Makes this simpler though. I added this to the new patch.


================
Comment at: libcxx/include/map:915
+
+    __base& __get_tree() { return __tree_; }
+
----------------
ldionne wrote:
> 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.
Oh, sorry! This was used in the implementation of `merge`, which I moved to a follow-up. I'll sink it down there and add the visibility attribute.


================
Comment at: libcxx/include/set:399
 
+template <class _Key, class _Compare, class _Allocator>
+class multiset;
----------------
ldionne wrote:
> Is this forward declaration necessary?
No, nice catch! This was also supposed to be a part of the follow-up, I'll move it there.


================
Comment at: libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_extract.pass.cpp:14
+
+// class unordered_map
+
----------------
ldionne wrote:
> 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.
Okay, the new patch divides up the tests for each container into 4 files: `extract_iterator.pass.cpp`, `extract_key.pass.cpp`, `insert_node_type.pass.cpp`, and `insert_node_type_hint.pass.cpp`. I decided to move the insert_return_type and node_type testing to `container.node/node_handle.pass.cpp` because they don't really depend on any specific container, and it matches how the standard defines these.


================
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;
----------------
ldionne wrote:
> Consider using `using XXX = YYY` consistently.
done!


================
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;
----------------
ldionne wrote:
> 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.
Sure, sorry! clang-format adds these in because it isn't smart enough to know when we're in a C++11 context.


================
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());
----------------
ldionne wrote:
> Why is this test useful?
I guess it isn't really, it was more of a sanity check.


https://reviews.llvm.org/D46845