Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Add missing xlocale.h include on Apple and FreeBSD #99689

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Jul 19, 2024

The <locale> header uses strtoll_l and friends which are defined in <xlocale.h> on these platforms. While this works via transitive includes when modules are disabled, this doesn't work anymore if the platforms are modularized properly.

The <locale> header uses strtoll_l and friends which are defined
in <xlocale.h> on these platforms. While this works via transitive
includes when modules are disabled, this doesn't work anymore if
the platforms are modularized properly.
@ldionne ldionne requested a review from var-const July 19, 2024 19:31
@ldionne ldionne requested a review from a team as a code owner July 19, 2024 19:31
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 19, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 19, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

The <locale> header uses strtoll_l and friends which are defined in <xlocale.h> on these platforms. While this works via transitive includes when modules are disabled, this doesn't work anymore if the platforms are modularized properly.


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

1 Files Affected:

  • (modified) libcxx/include/locale (+4)
diff --git a/libcxx/include/locale b/libcxx/include/locale
index dbec23a2c936d..573910a85bef5 100644
--- a/libcxx/include/locale
+++ b/libcxx/include/locale
@@ -232,6 +232,10 @@ template <class charT> class messages_byname;
 #    include <__locale_dir/locale_base_api/bsd_locale_fallbacks.h>
 #  endif
 
+#  if defined(__APPLE__) || defined(__FreeBSD__)
+#    include <xlocale.h>
+#  endif
+
 #  if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #    pragma GCC system_header
 #  endif

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.

We should export this from locale_base_api.h. That's what the header is for.

@ldionne
Copy link
Member Author

ldionne commented Jul 23, 2024

Even with export * in locale_base_api.h, we are unable to fix this issue from locale_base_api.h. I don't know why that is the case, but I would like to merge this patch so as to make libc++ 19 compatible with a modularized SDK while we spend some cycles cleaning up the <locale> story in parallel.

@ldionne ldionne added this to the LLVM 19.X Release milestone Jul 23, 2024
@philnik777
Copy link
Contributor

Even with export * in locale_base_api.h, we are unable to fix this issue from locale_base_api.h. I don't know why that is the case, but I would like to merge this patch so as to make libc++ 19 compatible with a modularized SDK while we spend some cycles cleaning up the <locale> story in parallel.

Could the problem be that we don't actually include the base API in <locale>?

@ldionne
Copy link
Member Author

ldionne commented Jul 25, 2024

No, I tried that and it doesn't help.

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.

OK, in that case let's ship this now and find a proper fix later.

@ldionne ldionne merged commit a55df23 into llvm:main Jul 25, 2024
57 checks passed
@ldionne ldionne deleted the review/xlocale-modules branch July 25, 2024 17:16
@ldionne
Copy link
Member Author

ldionne commented Jul 25, 2024

/cherry-pick a55df23

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 25, 2024
The `<locale>` header uses `strtoll_l` and friends which are defined in
`<xlocale.h>` on these platforms. While this works via transitive
includes when modules are disabled, this doesn't work anymore if the
platforms are modularized properly.

(cherry picked from commit a55df23)
@llvmbot
Copy link
Member

llvmbot commented Jul 25, 2024

/pull-request #100604

yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
The `<locale>` header uses `strtoll_l` and friends which are defined in
`<xlocale.h>` on these platforms. While this works via transitive
includes when modules are disabled, this doesn't work anymore if the
platforms are modularized properly.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250520
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 26, 2024
The `<locale>` header uses `strtoll_l` and friends which are defined in
`<xlocale.h>` on these platforms. While this works via transitive
includes when modules are disabled, this doesn't work anymore if the
platforms are modularized properly.

(cherry picked from commit a55df23)
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
Development

Successfully merging this pull request may close these issues.

4 participants