[libcxx-commits] [PATCH] D112660: [libc++] Use addressof in forward_list.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante planned changes to this revision.
Mordante marked an inline comment as done.
Mordante added inline comments.


================
Comment at: libcxx/include/forward_list:183
 #include <__config>
 #include <__utility/forward.h>
 #include <algorithm>
----------------
Quuxplusone wrote:
> Consider adding `#include <__memory/addressof.h>` here.
We already include `<memory>` at line 188 which should provide `addressof`.


================
Comment at: libcxx/test/std/containers/sequences/forwardlist/forwardlist.ops/merge_pred.addressof.compile.pass.cpp:22
+  std::forward_list<operator_hijacker> l;
+  l.merge(l, std::less<operator_hijacker>());
+}
----------------
Quuxplusone wrote:
> The comment on line 11 is talking about `forward_list&& x`, but here you're passing lvalue `l`. Which is correct? And should there be two test cases, one for lvalues and one for rvalues?
I copy pasted this from the existing test, which has the same issue. Having an rvalue in the comment and using an lvalue. It seems there are no tests for rvalues.

Looking at the implementation of `forward_list` all 4 merge functions were affected by this issue.
```
void merge(forward_list& x);
void merge(forward_list&& x);
template<class Compare> void merge(forward_list& x, Compare comp);
template<class Compare> void merge(forward_list&& x, Compare comp);
```

I'll fix this test and add the 3 missing `addressof` compile tests.
The missing runtime tests will be done in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112660