This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Mark everything in <deque> as _LIBCPP_HIDE_FROM_ABI and replace _LIBCPP_INLINE_VISIBILITY
ClosedPublic

Authored by philnik on Aug 20 2022, 5:03 PM.

Diff Detail

Event Timeline

philnik created this revision.Aug 20 2022, 5:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 5:03 PM
philnik requested review of this revision.Aug 20 2022, 5:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 5:03 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante requested changes to this revision.Aug 21 2022, 7:16 AM

I only skimmed the file and see no direct oddities.
But this patch is an ABI break on multiple platforms.

This revision now requires changes to proceed.Aug 21 2022, 7:16 AM

I only skimmed the file and see no direct oddities.
But this patch is an ABI break on multiple platforms.

It's not an ABI break. I added that test in the parent commit and didn't get the numbers right yet.

I only skimmed the file and see no direct oddities.
But this patch is an ABI break on multiple platforms.

It's not an ABI break. I added that test in the parent commit and didn't get the numbers right yet.

It wasn't clear from this patch that it was put on top of a "broken" patch. It would have been nice to add that as a hint for the reviewers.

I only skimmed the file and see no direct oddities.
But this patch is an ABI break on multiple platforms.

It's not an ABI break. I added that test in the parent commit and didn't get the numbers right yet.

It wasn't clear from this patch that it was put on top of a "broken" patch. It would have been nice to add that as a hint for the reviewers.

Sorry it wasn't clear. I thought the Stack (1 Open) makes it quite obvious.

It wasn't clear from this patch that it was put on top of a "broken" patch. It would have been nice to add that as a hint for the reviewers.

Sorry it wasn't clear. I thought the Stack (1 Open) makes it quite obvious.

I don't expect new patches on top of patches that don't compile, since that makes them fail too.
But the patch needs to be rebased anyway after the lower patch works again/is landed so will look at it then.

ldionne accepted this revision.Aug 25 2022, 9:26 AM

LGTM after rebase and green CI.

@Mordante I hope you don't mind me landing this without your approval.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 27 2022, 1:01 PM
This revision was automatically updated to reflect the committed changes.

@Mordante I hope you don't mind me landing this without your approval.

No problem.