[libcxx-commits] [PATCH] D112654: [libc++] Use addressof in list.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/list:185
 #include <__debug>
 #include <__utility/forward.h>
 #include <algorithm>
----------------
Quuxplusone wrote:
> Consider adding `#include <__memory/addressof.h>` here.
We already include `<memory>` at line 190 which should provide `addressof`.


================
Comment at: libcxx/test/std/containers/sequences/list/list.modifiers/emplace.addressof.compile.pass.cpp:24
+  std::list<operator_hijacker> l;
+  l.emplace(l.begin(), l.front());
+}
----------------
Quuxplusone wrote:
> Here `l.front()` is UB. Is it safe to just do `l.emplace(l.begin())` instead, or does that not trigger the codepath you're interested in?
> ...ah, I see this is a compile-only test... but still, let's avoid the avoidable UB, here and throughout.
> Alternatively, change this to
> ```
> void test(std::list<operator_hijacker>& l) {
>   l.emplace(l.begin(), l.front());
> }
> ```
> 
> Ditto on at least four test files below.
I like your suggestion to make the list an argument of the function!


================
Comment at: libcxx/test/std/containers/sequences/list/list.ops/merge_comp.addressof.compile.pass.cpp:12
+// template <class Compare> void merge(list& x, Compare comp);
+// If (&addressof(x) == this) does nothing; otherwise ...
+
----------------
Quuxplusone wrote:
> Stray `&` here. But this comment line seems unnecessary.
It's copied from another test. I'll remove the `&`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112654