This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] locale portability refactor
AbandonedPublic

Authored by bcraig on Feb 11 2016, 9:05 AM.

Details

Summary

Note: This was tested on Linux x86_64 with a host compiler of Clang 3.4 and a glibc standard library. Attempts were made to test on Cygwin, but it was pretty severely broken prior to this patch. It is likely that I have accidentally broken other OSes and C libraries. I'm open to recommendations on ways to test this that are better than "Submit, watch build bots, revert as necessary".

This patch attempts to consolidate the handling of *_l and posix locale management functions into the support headers. Outside of the support headers, _CXX_* versions of the functions will be called instead. _CXX_ was chosen as the prefix as it is a reserved name for the "implementation", so we won't conflict with user names. Double underscores were avoided as a prefix because some C libraries already use double underscores for internal versions of these functions and structures (e.g. GLIBC has an __locale_t, so we use _CXX_locale_t instead).

This patch also adds a configuration option that I see being useful for embedded users. Setting LIBCXX_ENABLE_LOCALE_EXTENSIONS to OFF will stub out a lot of the locale functionality in an inline friendly way.

As a consequence of fixing the locale portability headers, some other support headers got rearranged. Notably, monolothic support headers got split along functionality lines, and helpers for __builtin_* replacements got moved around as well.

Diff Detail

Event Timeline

bcraig updated this revision to Diff 47655.Feb 11 2016, 9:05 AM
bcraig retitled this revision from to [libcxx] locale portability refactor.
bcraig updated this object.
bcraig added subscribers: cfe-commits, joerg, jfb, scshunt.
bcraig updated this object.Feb 11 2016, 9:05 AM
bcraig updated this object.
bcraig updated this object.Feb 11 2016, 9:08 AM
bcraig updated this object.
mclow.lists edited edge metadata.Feb 11 2016, 11:20 AM

I am very much in favor of cleaning up this part of libc++.
Thanks for taking this on.

I'm open to recommendations on ways to test this that are better than "Submit, watch build bots, revert as necessary".

My suggestion is to do this incrementally.
Implement the infrastructure bits, then move (say) Linux over to use the new bits.
Then (say) Mac OS X. Then FreeBSD. Then IBM. Then Windows.
Repeat until everyone's moved over; watching the testbots (and getting the people who committed support for each of these systems involved).
Then, when they're all moved, delete the old stuff.

I'm open to recommendations on ways to test this that are better than "Submit, watch build bots, revert as necessary".

My suggestion is to do this incrementally.
Implement the infrastructure bits, then move (say) Linux over to use the new bits.
Then (say) Mac OS X. Then FreeBSD. Then IBM. Then Windows.
Repeat until everyone's moved over; watching the testbots (and getting the people who committed support for each of these systems involved).
Then, when they're all moved, delete the old stuff.

I can see some places where I can improve the reviewability of the code by doing things incrementally. Basically, I can shuffle code around in a few batches of changes first without substantially modifying the code in a few sets of changes. Then I can do the big _CXX_ transformation after that. I will see about opening different reviews for those smaller renames / shuffles, and link back to this review as justification.

I am hesitant to take the approach that you've described (port one platform at a time). I think that would make locale.cpp and <locale> have lots of code that looks like the following...

#if _USE_THE_NEW_STUFF
    return _CXX_btowc_l(c, __l);
#elif _LIBCPP_LOCALE__L_EXTENSIONS
    return btowc_l(c, __l);
#else
    return __btowc_l(c, __l);
#endif

With this approach, I'd still be doing a lot of blind submissions, and in the short term, the code would get worse. If, for whatever reason, I'm unable to complete the port and delete all the old stuff, then the refactor turns into a net negative.

bcraig abandoned this revision.Feb 22 2016, 7:38 AM

Breaking this up into smaller chunks.

include/support/xlocale/__nop_locale_mgmt.h