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

Tue Oct 3 11:37:24 PDT 2023

dblaikie 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
----------------
ldionne wrote:
> 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.
Fair - yeah, if I were maintaining only one version and I could write the union one, I would. But if it comes to maintaining two versions - I probably wouldn't bother with two versions for that benefit. *shrug*

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101206