[libcxx-commits] [PATCH] D101206: [libc++] Remove UB in list, forward_list and __hash_table

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 13 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/__hash_table:1394
         __node_traits::destroy(__na, _NodeTypes::__get_ptr(__real_np->__value_));
+#ifdef _LIBCPP_HASH_TABLE_PROPER_NODE_LIFETIME
+        std::__destroy_at(std::addressof(*__real_np));
----------------
EricWF wrote:
> EricWF wrote:
> > I don't know that we need to call the destructor here.
> > Do we?
> > 
> > 
> Lets drop the calls to the destructor. They should have no affect, and so it's more readable without them as it makes clear they don't perform any user-observable behaviors. 
As discussed during live review, we're going to call the destructor for symmetry with calling `construct_at`.


================
Comment at: libcxx/include/__hash_table:2235
+#ifdef _LIBCPP_HASH_TABLE_PROPER_NODE_LIFETIME
+    std::__construct_at(std::addressof(*__h), /* next = */nullptr, /* hash = */0);
+#else
----------------
EricWF wrote:
> EricWF wrote:
> > EricWF wrote:
> > > Why are we initializing more in this branch than we are in the other?
> > > 
> > > I would rather keep the two sets of behavior as convergent as possible.
> > In fact, why not simply make the constructor an nop, then keep using the code below to initialize the members.
> > 
> > It's easier to reason about one conditional compilation block than it is two.
> Same note as above. Simply default construct the object, then initialize it the same way we were.
As discussed during live review, the new approach will initialize the node in all standard modes.


================
Comment at: libcxx/include/forward_list:330
+    // Allow starting the lifetime of nodes without initializing the value held by the node.
+    // Note that we can't do that in C++03 because unions can't contain non-trivially constructible types.
+#ifndef _LIBCPP_CXX03_LANG
----------------
EricWF wrote:
> Is there a correctness reason for using the union over some other form of aligned storage?
As discussed, I think the main benefit is readability and probably the debugging experience in non-C++03 mode.


================
Comment at: libcxx/include/list:349
+    _LIBCPP_HIDE_FROM_ABI explicit __list_node(__link_pointer __prev, __link_pointer __next) : __base(__prev, __next) {}
+    _LIBCPP_HIDE_FROM_ABI ~__list_node() {}
+
----------------
EricWF wrote:
> Do  we cause ourselves any problems not starting the lifetime of the object in C++03 mode now that it's got a user defined destructor?
> 
> 
> I suspect not, because it's not like the type was always trivially destructible, but maybe?
> 
This will become moot with the new approach we discussed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101206/new/

https://reviews.llvm.org/D101206