[PATCH] D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge

[Louis Dionne via Phabricator]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM after rewording the comment. Please do not forget to submit a follow-up for the optimization.

Thanks for the great work and for the patience in getting this reviewed.



================
Comment at: libcxx/include/__hash_table:1356
         return const_local_iterator(nullptr, __n, bucket_count());
 #endif
     }
----------------
Please reword this comment in terms of "assumes that rehashing has already occurred and that no element with the same key exists in the map" (or something like that). Don't assume people have called `__node_insert_unique_prepare` before -- this sort of comment can get stale.


================
Comment at: libcxx/include/__hash_table:2356
+            __node_insert_multi_prepare(__src_hash, __src_ptr->__value_);
+        (void)__source.remove(__it++).release();
+        __src_ptr->__hash_ = __src_hash;
----------------
erik.pilkington wrote:
> ldionne wrote:
> > Same comment as above for `__it++`.
> I don't think that `__source.remove(__it).release(); ++it;` works here because after `__source.remove(__it)`, `__it` is no longer a valid iterator, so you can't increment it.
Good catch.


================
Comment at: libcxx/include/__tree:2500
+        ++__i;
+        if (__child != nullptr)
+            continue;
----------------
erik.pilkington wrote:
> ldionne wrote:
> > Do I understand correctly that `__child != nullptr` if and only if the key is already in `*this`, in which case we `continue` to skip the insertion (because that is a container with unique keys)?
> > 
> > The comment in `__find_equal` says
> > 
> > ```
> > // Find place to insert if __v doesn't exist
> > // Set __parent to parent of null leaf
> > // Return reference to null leaf
> > // If __v exists, set parent to node of __v and return reference to node of __v
> > ```
> > 
> > It looks like the "more correct" check to see whether the key already exists in `*this` would have been ` if (__child == __parent)` -- is that correct?
> > 
> > Do I understand correctly that __child != nullptr if and only if the key is already in *this, in which case we continue to skip the insertion (because that is a container with unique keys)?
> 
> Yep, exactly.
> 
> > It looks like the "more correct" check to see whether the key already exists in *this would have been  if (__child == __parent) -- is that correct?
> `__child == __parent` and `__child != nullptr` should be equivalent. It looks like other users of __find_equal check `__child != nullptr`, why do you say that `__child == __parent` is "more correct"?
I found it matched the comment in `__find_equal` more closely, but that's subjective. Leave as-is.


================
Comment at: libcxx/include/__tree:2562
+        __insert_node_at(__parent, __child,
+                         static_cast<__node_base_pointer>(__src_ptr));
+    }
----------------
erik.pilkington wrote:
> ldionne wrote:
> > Regarding static casting from `__node_pointer` to `__node_base_pointer` -- is this actually guaranteed to work for fancy pointer types? I don't think it is -- I think only casting to `void_ptr` and then back to `__node_base_pointer` would work per the `Allocator` concept requirements.
> > 
> > Note that in practice, a fancy pointer that would not support that would be misbehaved. Also, this problem already exists elsewhere in the code. I'm asking just for curiosity, I don't expect you to act on this comment in any way.
> > 
> Yep, I think you're analysis is right. Every (fancy) pointer conversion needs to go through `void`, but this doesn't. We actually do this a lot in this file. Do you think this actually matters in practice? Maybe @mclow.lists @EricWF have some thoughts?
It probably doesn't matter in practice. My experience using fancy pointers is that none of the standard libraries work with them. Only Boost.Container does.


https://reviews.llvm.org/D48896