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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Minor test nits, but a fair number of nits.



================
Comment at: libcxx/include/list:185
 #include <__debug>
 #include <__utility/forward.h>
 #include <algorithm>
----------------
Consider adding `#include <__memory/addressof.h>` here.


================
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());
+}
----------------
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.


================
Comment at: libcxx/test/std/containers/sequences/list/list.modifiers/insert_iter_size_value.addressof.compile.pass.cpp:13
+
+// Validate whether the container can be copy-assigned with an ADL-hijacking operator&
+
----------------
The comment on line 14 is mis-copy-and-pasted: this is not copy-assignment.
Ditto in many of these test files.


================
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 ...
+
----------------
Stray `&` here. But this comment line seems unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112654