This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Omit unneeded locale fallbacks on Android 21+
ClosedPublic

Authored by smeenai on Nov 7 2019, 8:02 PM.

Details

Summary

Android API level 21 and above have all these functions available, so we
don't need to include our fallback definitions.

Event Timeline

smeenai created this revision.Nov 7 2019, 8:02 PM
enh added a comment.Nov 7 2019, 9:27 PM

honestly, if it was me, i'd keep all these inlines even for current releases: inlining a call to the underlying function saves you a layer of useless cruft at runtime. (the Android "implementations" are just one-liners that drop the extra argument and call the underlying function, just like these inlines.)

if Android were the only system making use of this and you could otherwise remove it, that would be different, but since you need it for other configurations anyway...

In D69983#1738155, @enh wrote:

honestly, if it was me, i'd keep all these inlines even for current releases: inlining a call to the underlying function saves you a layer of useless cruft at runtime. (the Android "implementations" are just one-liners that drop the extra argument and call the underlying function, just like these inlines.)

if Android were the only system making use of this and you could otherwise remove it, that would be different, but since you need it for other configurations anyway...

In general, I feel it's cleaner to delegate to the actual system implementations wherever possible, in case there's additional logic (now or in the future) that gets omitted because of unnecessary wrapping. In particular, if the Android implementations are just one-liners too, they could just be inline in the NDK headers, and then you'd get the inlining regardless. Including the fallbacks unconditionally also means people will think of them as always used for Android, so in case they end up being unused on other platforms in the future, they still wouldn't be removed because of the unconditional Android inclusion.

To be perfectly honest, I have a particular internal setup where these definitions end up clashing with other definitions that are (correctly) keyed on API level >= 21, and removing the unnecessary fallbacks from libc++ is the easiest way to fix it. I certainly don't think libc++ should be burdened with supporting odd internal configurations in general, but in this particular case, restricting libc++'s own fallbacks only to where they're needed and letting the system handle the rest seems cleaner in general.

enh added a comment.Nov 11 2019, 7:55 AM

actually, it looks like clang is inlining the code for all the is*_l
functions anyway. you're paying for the indirection on
strcoll_l/strxfrm_l/wcscoll_l/wcsxfrm_l, but no one should be using
those anyway.

so fine by me...

Thanks. WIll wait for an approval from a libc++ code owner before committing.

This revision is now accepted and ready to land.Nov 25 2019, 11:04 AM
This revision was automatically updated to reflect the committed changes.