[libcxx-commits] [PATCH] D69983: [libcxx] Omit unneeded locale fallbacks on Android 21+

[Shoaib Meenai via libcxx-commits]

Thanks! Do you mind LGTMing on Phabricator as well, so that the review thread there is up-to-date?

On 11/8/19, 11:55 AM, "enh" <enh at google.com> wrote:

    On Fri, Nov 8, 2019 at 11:53 AM Shoaib Meenai <smeenai at fb.com> wrote:
    >
    > To clarify, are you then okay with conditionalizing the fallbacks in libc++ to Android API < 21, since Clang is inlining them anyway?
    
    yes, since this won't actually cause anyone to have worse code, the
    change seems fine to me.
    
    > Also, your email reply didn't make its way to Phabricator. I thought that had been fixed, but it didn't work in this particular case.
    >
    > On 11/8/19, 10:04 AM, "libcxx-commits on behalf of enh via libcxx-commits" <libcxx-commits-bounces at lists.llvm.org on behalf of libcxx-commits at lists.llvm.org> wrote:
    >
    >     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.
    >
    >     On Fri, Nov 8, 2019 at 9:49 AM Shoaib Meenai via Phabricator
    >     <reviews at reviews.llvm.org> wrote:
    >     >
    >     > smeenai added a comment.
    >     >
    >     > In D69983#1738155 <https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D69983-231738155&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=SpVGnSBkcD8tzkl64Msu7svNUmsElJw5p_5GP0PU6dM&s=jGHq-VCyteT1X_8V9XHNe28vUke4OMAdh5yPBCGBz8Y&e= >, @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.
    >     >
    >     >
    >     > Repository:
    >     >   rG LLVM Github Monorepo
    >     >
    >     > CHANGES SINCE LAST ACTION
    >     >   https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D69983_new_&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=SpVGnSBkcD8tzkl64Msu7svNUmsElJw5p_5GP0PU6dM&s=R8NLN7qThC8fDLnoJ5qSqR8mLJRidJQt3eUDugG_0ek&e=
    >     >
    >     > https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D69983&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=SpVGnSBkcD8tzkl64Msu7svNUmsElJw5p_5GP0PU6dM&s=IutSb9MHYgF-3s0RhltPK-AOkrhfqaBY_j7OCONdNv0&e=
    >     >
    >     >
    >     >
    >     _______________________________________________
    >     libcxx-commits mailing list
    >     libcxx-commits at lists.llvm.org
    >     https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_libcxx-2Dcommits&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=SpVGnSBkcD8tzkl64Msu7svNUmsElJw5p_5GP0PU6dM&s=qsWlnNi-3rb88EOjwkyD5UK84QiCBh9Fa2SDgRPzObs&e=
    >
    >