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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
