This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Fixes formatting vector<bool>
ClosedPublic

Authored by Mordante on Feb 17 2023, 10:08 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG7e4639d28f44: [libc++][format] Fixes formatting vector<bool>
Summary

Formatting a const qualified vector<bool> fails to work on libc++. This
is due to our non-conforming type for vector<bool>::const_reference. The
type is a __bit_const_reference<vector> instead of a bool. (This is
fixed in ABI v2.)

This fixes this formatter and enables the test written for it.

Diff Detail

Event Timeline

Mordante created this revision.Feb 17 2023, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 10:08 AM
Mordante requested review of this revision.Feb 17 2023, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 10:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.
libcxx/include/__bit_reference
158

Why not just _Cp?

Mordante added inline comments.Feb 22 2023, 9:19 AM
libcxx/include/__bit_reference
158

To make sure this is used with a std::vector, similar to the friend at line 155.

philnik added inline comments.Feb 22 2023, 9:28 AM
libcxx/include/__bit_reference
158

How would it not be used with a std::vector or std::bitset? If a user tries to instantiate it with another class it's not exactly hard to add the alias.

ldionne accepted this revision.Mar 3 2023, 1:23 PM
ldionne added a subscriber: ldionne.

LGTM

libcxx/include/__bit_reference
158

FWIW I think the __self thing is really weird, but I would support changing that in its own patch if we want to do something about it.

This revision is now accepted and ready to land.Mar 3 2023, 1:23 PM
Mordante marked 3 inline comments as done.Mar 4 2023, 4:59 AM

Thanks for the reviews!

libcxx/include/__bit_reference
158

How would it not be used with a std::vector or std::bitset? If a user tries to instantiate it with another class it's not exactly hard to add the alias.

Of course you can, but you're not allowed to since it uses the ugly name.

FWIW I think the __self thing is really weird, but I would support changing that in its own patch if we want to do something about it.

I don't feel to strongly about it, so I will not pursue it. (I've more things to do, which I deem important.)

158

FWIW I think the __self thing is really weird, but I would support changing that in its own patch if we want to do something about it.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.