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

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai added a comment.

In D69983#1738155 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69983/new/

https://reviews.llvm.org/D69983