Skip to content

Conversation

@enh-google
Copy link
Contributor

No description provided.

@enh-google enh-google requested a review from a team as a code owner July 15, 2025 16:09
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 15, 2025
@enh-google enh-google requested a review from brad0 July 15, 2025 16:09
@llvmbot
Copy link
Member

llvmbot commented Jul 15, 2025

@llvm/pr-subscribers-libcxx

Author: None (enh-google)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/148879.diff

2 Files Affected:

  • (modified) libcxx/include/__locale_dir/locale_base_api.h (-2)
  • (removed) libcxx/include/__locale_dir/locale_base_api/android.h (-45)
diff --git a/libcxx/include/__locale_dir/locale_base_api.h b/libcxx/include/__locale_dir/locale_base_api.h
index 8dbc28e839839..9f3ce02a3af20 100644
--- a/libcxx/include/__locale_dir/locale_base_api.h
+++ b/libcxx/include/__locale_dir/locale_base_api.h
@@ -129,8 +129,6 @@
 //       will define those directly.
 #    if defined(_AIX) || defined(__MVS__)
 #      include <__locale_dir/locale_base_api/ibm.h>
-#    elif defined(__ANDROID__)
-#      include <__locale_dir/locale_base_api/android.h>
 #    elif defined(__OpenBSD__)
 #      include <__locale_dir/locale_base_api/openbsd.h>
 #    elif defined(__wasi__) || _LIBCPP_HAS_MUSL_LIBC
diff --git a/libcxx/include/__locale_dir/locale_base_api/android.h b/libcxx/include/__locale_dir/locale_base_api/android.h
deleted file mode 100644
index 36b8d93e1b228..0000000000000
--- a/libcxx/include/__locale_dir/locale_base_api/android.h
+++ /dev/null
@@ -1,45 +0,0 @@
-// -*- C++ -*-
-//===-----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef _LIBCPP___LOCALE_DIR_LOCALE_BASE_API_ANDROID_H
-#define _LIBCPP___LOCALE_DIR_LOCALE_BASE_API_ANDROID_H
-
-#include <stdlib.h>
-
-// FIXME: Is this actually required?
-extern "C" {
-#include <xlocale.h>
-}
-
-#include <android/api-level.h>
-
-// If we do not have this header, we are in a platform build rather than an NDK
-// build, which will always be at least as new as the ToT NDK, in which case we
-// don't need any of the inlines below since libc provides them.
-#if __has_include(<android/ndk-version.h>)
-#  include <android/ndk-version.h>
-// In NDK versions later than 16, locale-aware functions are provided by
-// legacy_stdlib_inlines.h
-#  if __NDK_MAJOR__ <= 16
-#    if __ANDROID_API__ < 26
-
-inline _LIBCPP_HIDE_FROM_ABI float strtof_l(const char* __nptr, char** __endptr, locale_t) {
-  return ::strtof(__nptr, __endptr);
-}
-
-inline _LIBCPP_HIDE_FROM_ABI double strtod_l(const char* __nptr, char** __endptr, locale_t) {
-  return ::strtod(__nptr, __endptr);
-}
-
-#    endif // __ANDROID_API__ < 26
-
-#  endif // __NDK_MAJOR__ <= 16
-#endif   // __has_include(<android/ndk-version.h>)
-
-#endif // _LIBCPP___LOCALE_DIR_LOCALE_BASE_API_ANDROID_H

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move to the new base API since we're touching things anyways? I think it should be as simple as including <__locale_dir/support/bsd_like.h>.

@enh-google
Copy link
Contributor Author

Could we move to the new base API since we're touching things anyways? I think it should be as simple as including <__locale_dir/support/bsd_like.h>.

where? i was assuming that the

#  elif defined(__linux__)
#    include <__locale_dir/support/linux.h>

already in the file is the expected path for Android (since we'll already have been taking that anyway!).

@ldionne
Copy link
Member

ldionne commented Jul 15, 2025

#  elif defined(__linux__)
#    include <__locale_dir/support/linux.h>

already in the file is the expected path for Android (since we'll already have been taking that anyway!).

If Android is already taking the __linux__ path, then IMO this change makes sense as-is.

@enh-google
Copy link
Contributor Author

If Android is already taking the __linux__ path, then IMO this change makes sense as-is.

yeah, the other copy of this file (which i was going to clean up separately, so let me know if you'd prefer to have all the changes in one PR) doesn't have a __linux__ case: https://github.com/llvm/llvm-project/blob/8612926c306c5191a5fb385dd11467728c59e982/libcxx/include/__cxx03/__locale_dir/locale_base_api.h

so this code is actually still used in that copy (and in older versions of this copy), but it looks like your recent cleanup (363bfd6) accidentally made this dead code --- but that was the right thing to do anyway, so no-one noticed :-)

(i'm only here because brad0 has been cleaning up various "we don't need that any more" Android hacks around the tree, and this was the only place i found that was testing for specific versions of the NDK, not because this caused any trouble.)

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for explaining. Don't make any changes to __cxx03/*, these headers are frozen and shouldn't be modified (at least, not right now).

LGTM with green CI. Let's also give some time for @philnik777 to chime in again, but I think this should be pretty uncontroversial. Thanks a lot for the cleanup, this is appreciated!

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize Android defined __linux__. LGTM! I'm always happy when we can simplify our code.

@enh-google
Copy link
Contributor Author

Got it, thanks for explaining. Don't make any changes to __cxx03/*, these headers are frozen and shouldn't be modified (at least, not right now).

ack. let me know if/when that changes and you'd like me to clean up the mess.

LGTM with green CI. Let's also give some time for @philnik777 to chime in again, but I think this should be pretty uncontroversial. Thanks a lot for the cleanup, this is appreciated!

any time you see anything like this that you're curious about, feel free to reach out (i think you already know other Android folks too) ... no point leaving cruft lying around, and if it is non-obviously useful, we can at least try to add a better code comment to motivate it!

@enh-google enh-google merged commit 62bd778 into llvm:main Jul 16, 2025
77 checks passed
@enh-google enh-google deleted the no-ancient-ndk branch July 16, 2025 11:39
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jul 16, 2025

LLVM Buildbot has detected a new failure on builder premerge-monolithic-linux running on premerge-linux-1 while building libcxx,llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/38318

Here is the relevant piece of the build log for the reference
Step 7 (test-build-unified-tree-check-all) failure: test (failure)
...
PASS: lld :: COFF/comdat-jumptable.s (98562 of 101607)
PASS: lld :: COFF/code-merge.s (98563 of 101607)
PASS: lld :: COFF/could-not-open.test (98564 of 101607)
PASS: lld :: COFF/base.test (98565 of 101607)
PASS: lld :: COFF/cgprofile-print.s (98566 of 101607)
PASS: lld :: COFF/cl-gl.test (98567 of 101607)
PASS: lld :: COFF/comdat-gcc-compatibility-size.test (98568 of 101607)
PASS: lld :: COFF/comdat-drectve.s (98569 of 101607)
PASS: lld :: COFF/comdat-gcc-compatibility.s (98570 of 101607)
TIMEOUT: MLIR :: Examples/standalone/test.toy (98571 of 101607)
******************** TEST 'MLIR :: Examples/standalone/test.toy' FAILED ********************
Exit Code: 1
Timeout: Reached timeout of 60 seconds

Command Output (stdout):
--
# RUN: at line 1
"/etc/cmake/bin/cmake" "/build/buildbot/premerge-monolithic-linux/llvm-project/mlir/examples/standalone" -G "Ninja"  -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang  -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/build/buildbot/premerge-monolithic-linux/build/lib/cmake/mlir  -DLLVM_USE_LINKER=lld  -DPython3_EXECUTABLE="/usr/bin/python3.10"
# executed command: /etc/cmake/bin/cmake /build/buildbot/premerge-monolithic-linux/llvm-project/mlir/examples/standalone -G Ninja -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/build/buildbot/premerge-monolithic-linux/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE=/usr/bin/python3.10
# .---command stdout------------
# | -- The CXX compiler identification is Clang 16.0.6
# | -- The C compiler identification is Clang 16.0.6
# | -- Detecting CXX compiler ABI info
# | -- Detecting CXX compiler ABI info - done
# | -- Check for working CXX compiler: /usr/bin/clang++ - skipped
# | -- Detecting CXX compile features
# | -- Detecting CXX compile features - done
# | -- Detecting C compiler ABI info
# | -- Detecting C compiler ABI info - done
# | -- Check for working C compiler: /usr/bin/clang - skipped
# | -- Detecting C compile features
# | -- Detecting C compile features - done
# | -- Looking for histedit.h
# | -- Looking for histedit.h - found
# | -- Found LibEdit: /usr/include (found version "2.11") 
# | -- Found ZLIB: /usr/lib/x86_64-linux-gnu/libz.so (found version "1.2.11") 
# | -- Found LibXml2: /usr/lib/x86_64-linux-gnu/libxml2.so (found version "2.9.13") 
# | -- Using MLIRConfig.cmake in: /build/buildbot/premerge-monolithic-linux/build/lib/cmake/mlir
# | -- Using LLVMConfig.cmake in: /build/buildbot/premerge-monolithic-linux/build/lib/cmake/llvm
# | -- Linker detection: unknown
# | -- Performing Test LLVM_LIBSTDCXX_MIN
# | -- Performing Test LLVM_LIBSTDCXX_MIN - Success
# | -- Performing Test LLVM_LIBSTDCXX_SOFT_ERROR
# | -- Performing Test LLVM_LIBSTDCXX_SOFT_ERROR - Success
# | -- Performing Test CXX_SUPPORTS_CUSTOM_LINKER
# | -- Performing Test CXX_SUPPORTS_CUSTOM_LINKER - Success
# | -- Performing Test C_SUPPORTS_FPIC
# | -- Performing Test C_SUPPORTS_FPIC - Success
# | -- Performing Test CXX_SUPPORTS_FPIC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants