This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use unsigned char in basic_string::__short again
ClosedPublic

Authored by philnik on Jun 11 2022, 2:44 AM.

Details

Summary

D125496 changed the string layout on windows. Change __is_long_ and __size_ back to using unsigned char to fix the issue.

Diff Detail

Event Timeline

philnik created this revision.Jun 11 2022, 2:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2022, 2:44 AM
philnik requested review of this revision.Jun 11 2022, 2:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2022, 2:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision.Jun 11 2022, 11:33 AM

LGTM modulo one comment.

libcxx/include/string
697

As I already commented on D125496 I think we should add static_asserts validating the size. That should catch these errors in our CI.

(same for the one below.)

This revision is now accepted and ready to land.Jun 11 2022, 11:33 AM
jloser added a subscriber: jloser.Jun 11 2022, 11:55 AM
jloser added inline comments.
libcxx/include/string
697

I agree re binding this behavior with a test to avoid surprises again in the future.

philnik updated this revision to Diff 436164.Jun 11 2022, 1:06 PM
philnik marked 2 inline comments as done.
  • Add static_assert to verify the size of __short
philnik updated this revision to Diff 436168.Jun 11 2022, 2:43 PM
  • Add message to static_assert

It seems like AIX is currently broken. I'll land it for now. If this breaks AIX (which it shouldn't) we'll know and I can try to fix it then.

This revision was landed with ongoing or failed builds.Jun 12 2022, 12:06 PM
This revision was automatically updated to reflect the committed changes.

We need to add a test (in libcxx/test/libcxx/) to ensure that we can't regress this in the future.