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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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
----------------
dblaikie wrote:
> ldionne wrote:
> > 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.
> Readability seems a bit limited if the cost is `#ifdef`s and having the other implementation here too, though, yeah? Like this code would be easier, I think, to read and modify if there was only one implementation even if it's the aligned storage one, rather than having two. Less is easier to read, generally.
> 
> Debuggability - yeah, that's true, though probably not super important except for standard library authors? (& sounds like you're not convinced/certain it'd be especially helpful to you personally?) - if you want to inspect the value in the node directly, without invoking any code (eg: debugging a core dump) then the union is easier (you don't have to write out the cast in a debugger expression evaluator to get the value from the aligned storage)
> 
> (I'm not making some firm statement here - just my 2c, feel free to disregard)
I don't have a strong opinion, but I think the "inspecting a value from the debugger is easier with a union" was the main motivating factor for doing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101206