[libcxx-commits] [PATCH] D139497: [libc++][Android] Bionic also includes + for NAN

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

A few nits and I'm happy.



================
Comment at: libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp:10736
+#if defined(TEST_HAS_GLIBC) || defined(_WIN32) || \
+        (defined(__BIONIC__) && !defined(_LIBCPP_TESTING_ANDROID_PRINTF_NAN_NO_SIGN))
     std::string pnan_sign = "+";
----------------
rprichard wrote:
> Mordante wrote:
> > Would this work too?
> It ties the expected test output to the build API level of the test, which may not be what we want.
> 
> i.e. There are three API levels:
>  - the build API level of libc++ itself
>  - the build API of the libc++ tests (or the application code)
>  - the runtime API level of the device
> 
> Currently for the NDK, the libc++ build API is typically 19 or 21, the app build API is typically low-ish (maybe 21/23?), and the runtime API steadily increases as people buy newer devices. It's normal for the build API for application code to be low (to target all the old devices in the wild), and the NDK only packages a single `libc++_{shared.so,static.a}` per CPU architecture.
> 
> This use of `__ANDROID_API__` would break the test config in D139147, where the build API for libc++ and the tests is 21 (AndroidNDK.cmake,  llvm-libc++abi-android-ndk.cfg.in), but the runtime API is 33 (Dockerfile.android).
> 
I see, can you add the gist of this information to the comment in features.py.


================
Comment at: libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp:10736
+#if defined(TEST_HAS_GLIBC) || defined(_WIN32) || \
+        (defined(__BIONIC__) && !defined(_LIBCPP_TESTING_ANDROID_PRINTF_NAN_NO_SIGN))
     std::string pnan_sign = "+";
----------------
Mordante wrote:
> rprichard wrote:
> > Mordante wrote:
> > > Would this work too?
> > It ties the expected test output to the build API level of the test, which may not be what we want.
> > 
> > i.e. There are three API levels:
> >  - the build API level of libc++ itself
> >  - the build API of the libc++ tests (or the application code)
> >  - the runtime API level of the device
> > 
> > Currently for the NDK, the libc++ build API is typically 19 or 21, the app build API is typically low-ish (maybe 21/23?), and the runtime API steadily increases as people buy newer devices. It's normal for the build API for application code to be low (to target all the old devices in the wild), and the NDK only packages a single `libc++_{shared.so,static.a}` per CPU architecture.
> > 
> > This use of `__ANDROID_API__` would break the test config in D139147, where the build API for libc++ and the tests is 21 (AndroidNDK.cmake,  llvm-libc++abi-android-ndk.cfg.in), but the runtime API is 33 (Dockerfile.android).
> > 
> I see, can you add the gist of this information to the comment in features.py.
We typically only use `_LIBCPP` for configuration options in libc++ itself and not for the test suite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139497