Page MenuHomePhabricator

[libc++] Use addressof in forward_list.
ClosedPublic

Authored by Mordante on Oct 27 2021, 1:29 PM.

Details

Reviewers
Quuxplusone
ldionne
Group Reviewers
Restricted Project
Commits
rGf7345de64fd2: [libc++] Use addressof in forward_list.
Summary

This addresses the usage of operator& in <forward_list>.

(Note there are still more headers with the same issue.)

Diff Detail

Event Timeline

Mordante requested review of this revision.Oct 27 2021, 1:29 PM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2021, 1:29 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/forward_list
183

Consider adding #include <__memory/addressof.h> here.

libcxx/test/std/containers/sequences/forwardlist/forwardlist.ops/merge_pred.addressof.compile.pass.cpp
22 ↗(On Diff #382769)

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?

Mordante planned changes to this revision.Oct 27 2021, 2:03 PM
Mordante marked an inline comment as done.
Mordante added inline comments.
libcxx/include/forward_list
183

We already include <memory> at line 188 which should provide addressof.

libcxx/test/std/containers/sequences/forwardlist/forwardlist.ops/merge_pred.addressof.compile.pass.cpp
22 ↗(On Diff #382769)

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.

libcxx/include/forward_list
183

Yes, and certainly it's included transitively from a lot of places. Just trying to sneak in some granularization. :) No worries.

Mordante updated this revision to Diff 385346.Nov 7 2021, 8:06 AM
Mordante marked an inline comment as done.

Addresses review comments.

Mordante marked an inline comment as done.Nov 7 2021, 8:08 AM
Mordante updated this revision to Diff 385350.Nov 7 2021, 8:50 AM

Rebase to fix CI.

ldionne accepted this revision.Nov 8 2021, 1:25 PM
This revision is now accepted and ready to land.Nov 8 2021, 1:25 PM
This revision was automatically updated to reflect the committed changes.