The Android platform does not have ndk-version.h, but it will always
have up-to-date libc headers, so it does not need any compatibility
code intended for past versions of NDK_MAJOR. If ndk-version.h
is missing, assume NDK_MAJOR is (conceptually) infinite.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Question: This format is amended by linter automatically. Does it need to be changed back?
No, you can ignore the linter suggestions. You can actually use arc diff --nolint when uploading patches to avoid these warnings, which are not very relevant in libc++.
libcxx/include/__support/android/locale_bionic.h | ||
---|---|---|
32–43 | 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. |
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.
libcxx/include/__support/android/locale_bionic.h | ||
---|---|---|
27–28 | 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. | |
32–36 | We could simply remove this comment, because the 3-line comment below (in front of __has_include) already explains what's going on. | |
32–43 | 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. |
The link you provided seems to point to a private bug tracker.
Can you update the description of patch so it's clear what this tries to fix.
(I don't mind the private link, but I like to have some public info too.)
Hi,
Thank you for reminding me! Sorry for the inconvenience! Already updated
to the summary! If you have any questions, please let me know :D
Best,
Zijun
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.