[libcxx-commits] [PATCH] D122593: Fix CtsDeqpTestCases failures

[Ryan Prichard via Phabricator via libcxx-commits]

rprichard added a comment.

Suggested issue title: [libcxx] locale_bionic.h: skip ndk-version.h on Android platform

---

The CtsDeqpTestCases test failures aren't that closely related to this patch -- to fix them, we needed to upgrade the copy of the NDK libc++ prebuilts in the platform repository (i.e. the "NDK-in-platform use case" in the comment above), and to do that, we had to reapply this header file patch to account for the lack of an ndk-version.h in the platform build.

i.e. We've been reapplying this patch a few times, but it would be better to have it in upstream LLVM.



================
Comment at: libcxx/include/__support/android/locale_bionic.h:29
 #include <android/api-level.h>
 #include <android/ndk-version.h>
 #if __ANDROID_API__ < 21
----------------
This `#include` of ndk-version.h should be removed in favor of the one below.

I wonder how useful the api-level.h include here is. Mostly it just defines the various `__ANDROID_API_<xxx>__` macros, which we don't use here. It does ensure that `__ANDROID_API__` is defined, but normally Clang will have defined that as a built-in macro. Still, I think it's harmless, so I'd leave this api-level.h include alone.



================
Comment at: libcxx/include/__support/android/locale_bionic.h:34-38
+// HACK: Not in upstream NDK or libc++.
+// Upstream now supports using ToT libc++ with old NDKs, but as such it is now
+// *only* compatible with the NDK. That will need to be fixed both for the
+// platorm and for the NDK-in-platform use case since neither has
+// android/ndk-version.h.
----------------
rprichard wrote:
> ldionne wrote:
> > I don't really have a concern with this patch per se because it's self contained in bionic specific headers, however this doesn't really seem to be meant for upstreaming. At least, we should update the comment so that it makes more sense to carry upstream.
> This patch adds support for the Android-platform-but-not-NDK use case, so I think it makes sense to upstream. I agree that the comment (and title) needs fixing.
We could simply remove this comment, because the 3-line comment below (in front of `__has_include`) already explains what's going on.


================
Comment at: libcxx/include/__support/android/locale_bionic.h:34-45
+// HACK: Not in upstream NDK or libc++.
+// Upstream now supports using ToT libc++ with old NDKs, but as such it is now
+// *only* compatible with the NDK. That will need to be fixed both for the
+// platorm and for the NDK-in-platform use case since neither has
+// android/ndk-version.h.
+
+// If we do not have this header, we are in a platform build rather than an NDK
----------------
ldionne wrote:
> I don't really have a concern with this patch per se because it's self contained in bionic specific headers, however this doesn't really seem to be meant for upstreaming. At least, we should update the comment so that it makes more sense to carry upstream.
This patch adds support for the Android-platform-but-not-NDK use case, so I think it makes sense to upstream. I agree that the comment (and title) needs fixing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122593