This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use addressof in vector.
ClosedPublic

Authored by Mordante on Oct 17 2021, 5:28 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG56df1d80e291: [libc++] Use addressof in vector.
Summary

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

I now added tests for the current offending cases. I wonder whether it
would be better to add one addressof test per directory and test all
possible violations. Also to guard against possible future errors?

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

Diff Detail

Event Timeline

Mordante requested review of this revision.Oct 17 2021, 5:28 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2021, 5:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

LprettyGTM! You'll need to #include <__memory/addressof.h> in at least one, maybe two, places, before CI will be happy.

libcxx/include/vector
1740

Pre-existing: iterator, iterator should be iterator, iterator (but that'll require updating tests, and should be a separate commit anyway)

ldionne accepted this revision.Oct 18 2021, 8:17 AM
ldionne added a subscriber: ldionne.

I now added tests for the current offending cases. I wonder whether it would be better to add one addressof test per directory and test all possible violations.

I won't say "no" to additional test coverage, but that sounds like a lot of work for not necessarily that much benefit. If you want to do it, then sure, but there will be a ton of stuff to check.

Whether or not we decide to expand the testing of addressof, I'm fine with this patch. Thanks for tackling this!

[needs CI to pass, as usual]

This revision is now accepted and ready to land.Oct 18 2021, 8:17 AM
Mordante updated this revision to Diff 380713.Oct 19 2021, 9:35 AM

Adds a missing include.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.Oct 21 2021, 9:32 AM

For now I keep the minimal set of tests; there are still quite some headers to review left ;-)

libcxx/include/vector
1740

I created D112231, it seems not to affect tests.