This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use addressof in list.
ClosedPublic

Authored by Mordante on Oct 27 2021, 12:44 PM.

Details

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

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

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

Diff Detail

Event Timeline

Mordante requested review of this revision.Oct 27 2021, 12:44 PM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2021, 12:44 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Oct 27 2021, 1:43 PM
Quuxplusone added a subscriber: Quuxplusone.

Minor test nits, but a fair number of nits.

libcxx/include/list
185

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

libcxx/test/std/containers/sequences/list/list.modifiers/emplace.addressof.compile.pass.cpp
24

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.

libcxx/test/std/containers/sequences/list/list.modifiers/insert_iter_size_value.addressof.compile.pass.cpp
13

The comment on line 14 is mis-copy-and-pasted: this is not copy-assignment.
Ditto in many of these test files.

libcxx/test/std/containers/sequences/list/list.ops/merge_comp.addressof.compile.pass.cpp
12

Stray & here. But this comment line seems unnecessary.

This revision now requires changes to proceed.Oct 27 2021, 1:43 PM
Mordante marked 4 inline comments as done.Nov 7 2021, 9:30 AM
Mordante added inline comments.
libcxx/include/list
185

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

libcxx/test/std/containers/sequences/list/list.modifiers/emplace.addressof.compile.pass.cpp
24

I like your suggestion to make the list an argument of the function!

libcxx/test/std/containers/sequences/list/list.ops/merge_comp.addressof.compile.pass.cpp
12

It's copied from another test. I'll remove the &.

Mordante updated this revision to Diff 385353.Nov 7 2021, 9:31 AM
Mordante marked 3 inline comments as done.

Addresses review comments.

Seems reasonable to me.

libcxx/test/std/containers/sequences/list/list.modifiers/erase_iter.addressof.compile.pass.cpp
23–24

Nit: You don't do it = for insert; I don't think you need to do it for erase.

ldionne accepted this revision.Nov 8 2021, 1:23 PM
This revision is now accepted and ready to land.Nov 8 2021, 1:23 PM
Mordante marked an inline comment as done.Nov 11 2021, 9:55 AM
This revision was automatically updated to reflect the committed changes.