This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Change vector<bool>::const_iterator::reference to bool in ABIv2
ClosedPublic

Authored by philnik on Apr 15 2022, 4:21 AM.

Details

Summary

vector<bool>::const_reference and vector<bool>::const_iterator::reference should be the same type.

Diff Detail

Event Timeline

philnik created this revision.Apr 15 2022, 4:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 4:21 AM
philnik requested review of this revision.Apr 15 2022, 4:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 4:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 423262.Apr 16 2022, 1:49 PM
  • Try to fix CI

I see _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL isn't mentioned in the release notes, can you add an entry?

LGTM modulo some small nits.

libcxx/test/std/containers/sequences/vector.bool/iterators.pass.cpp
35

Interestingly the test libcxx/test/std/containers/sequences/vector.bool/types.pass.cpp isn't affected.

It would be nice when you can add the const_reference to the synopsis and add tests for pointer, const_pointer, and const_reference.

philnik updated this revision to Diff 424032.Apr 20 2022, 2:29 PM
philnik marked an inline comment as done.
  • Address comments
libcxx/test/std/containers/sequences/vector.bool/iterators.pass.cpp
35

I don't think there is anything portable to test for pointer and const_pointer.

Mordante accepted this revision.Apr 21 2022, 9:15 AM

LGTM with one suggestion.

libcxx/test/std/containers/sequences/vector.bool/types.pass.cpp
27

For consistency I would use a typedef here.

This revision is now accepted and ready to land.Apr 21 2022, 9:15 AM
This revision was landed with ongoing or failed builds.Apr 22 2022, 11:57 AM
This revision was automatically updated to reflect the committed changes.
philnik marked an inline comment as done.
miyuki added a subscriber: miyuki.Apr 25 2022, 5:09 AM
miyuki added inline comments.
libcxx/include/__bit_reference
1115

Hi. This line causes breakages when compiling with -std=c++03:

/include/libcxx/__bit_reference:1115:79: error: a space is required between consecutive right angle brackets (use '> >')
    using reference = typename conditional<_IsConst, bool, __bit_reference<_Cp>>::type;

                                                                                 ^

Could you please fix this?

philnik marked an inline comment as done.Apr 25 2022, 6:23 AM
philnik added inline comments.
libcxx/include/__bit_reference
1115

Fixed it in 6b257a. Just out of interest, why do you use the unstable ABI in C++03? It's clearly not for ABI reasons. Also as a side note: I don't think we officially support using the unstable ABI pre-C++11, so you should probably not use that combination. But the fix is simple enough.

miyuki added inline comments.Apr 25 2022, 7:42 AM
libcxx/include/__bit_reference
1115

Because we are an embedded toolchain vendor and ABI stability is less of a concern for us, so we use a custom subset of unstable ABI features (and _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL improves results of conformance tests). We do claim C++03 support, though.

Also as a side note: I don't think we officially support using the unstable ABI pre-C++11

I was not aware of this; thanks for pointing it out.

miyuki added inline comments.Apr 25 2022, 7:50 AM
libcxx/include/__bit_reference
1115

But the fix is simple enough.

Thanks.