This is an archive of the discontinued LLVM Phabricator instance.

Upstream Bionic definitions of ctype_base/regex.
ClosedPublic

Authored by danalbert on Mar 13 2020, 6:03 PM.

Details

Reviewers
EricWF
ldionne
Group Reviewers
Restricted Project
Commits
rGcbf1904a3e05: Upstream Bionic definitions of ctype_base/regex.
Summary

This is a patch that Android has been carrying in its tree for several
years. This patch upstreams the existing ABI.

There's some historical cruft here. __regex_word used to be a part of
regex_traits rather than ctype_base. Bionic also used to use its own
ctype implementation because the libc++ builtin one wasn't available
yet. Bionic's ctype masks were 8 bits wide and already saturated, so a
wider type needed to be used for the regex mask, and the existing
value was already used so Android needed to specify its own.

Since then Android has migrated to the builtin ctype implementation
and this patch probably should have been dropped then. Unfortunately
that was not noticed at the time, so now we need to keep this to
maintain the current ABI.

Diff Detail

Event Timeline

danalbert created this revision.Mar 13 2020, 6:03 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 13 2020, 6:03 PM
ldionne accepted this revision.Mar 25 2020, 9:15 AM
ldionne added a subscriber: ldionne.

I know I've requested it before privately, but I would really love to have publicly available builders that test this configuration. As it stands, no builder at all will test this, so from our perspective this is a no-op.

libcxx/include/__locale
502

At first I wasn't so warm on this change cause of the added complexity, and then I looked at the context around it and I screamed. It's already a mess of platform-specific choices, so I can't reasonably push back on this.

libcxx/include/regex
998

Do we specialize this class anywhere?

This revision is now accepted and ready to land.Mar 25 2020, 9:15 AM
danalbert marked 4 inline comments as done.Apr 6 2020, 1:32 PM
danalbert added inline comments.
libcxx/include/__locale
502

Yeah. I'd originally not upstreamed this because it's gross :(

libcxx/include/regex
998

Doesn't look like it.

This revision was automatically updated to reflect the committed changes.
danalbert marked 2 inline comments as done.