This is an archive of the discontinued LLVM Phabricator instance.

Differential D158669

[libc++][Modules] locale fails to compile with clang modules when _LIBCPP_LOCALE__L_EXTENSIONS is undefined
ClosedPublic

Authored by iana on Aug 23 2023, 1:52 PM.

Details

Reviewers
ldionne
Mordante
Group Reviewers
Restricted Project
Commits
rG6021c78fe55e: [libc++][Modules] locale fails to compile with clang modules when…
Summary

When __locale_dir/locale_base_api/locale_guard.h is compiled independently, as it is when it's in its own clang module, it fails to compile due to locale_t being undefined. It needs to include __locale to get that, instead of just clocale.

Diff Detail

Event Timeline

iana created this revision.Aug 23 2023, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 1:53 PM
Herald added a subscriber: ributzka. · View Herald Transcript
iana requested review of this revision.Aug 23 2023, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 1:53 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
iana edited the summary of this revision. (Show Details)Aug 23 2023, 1:53 PM

It looks like we don't have unit tests/CI for undef(_LIBCPP_HAS_NO_LOCALIZATION) + undef(_LIBCPP_LOCALE__L_EXTENSIONS). If anyone knows how to set that up please let me know!

Harbormaster completed remote builds in B254463: Diff 552879.Aug 23 2023, 8:11 PM
ldionne added inline comments.Aug 24 2023, 11:03 AM
libcxx/include/__locale_dir/locale_base_api/locale_guard.h
12–14

We still need <clocale> because we are using setlocale(...) and friends. But we should indeed add <__locale> because that is what defines locale_t when libcxx/include/__support/xlocale/__nop_locale_mgmt.h is used.

iana updated this revision to Diff 553200.Aug 24 2023, 11:04 AM

Put the clocale include back

libcxx/include/__locale_dir/locale_base_api/locale_guard.h
12–14

👍 put <clocale> back

iana marked an inline comment as done.Aug 24 2023, 11:05 AM
iana updated this revision to Diff 553202.Aug 24 2023, 11:06 AM

Add a comment why we're including __locale because it's not obvious

iana added inline comments.Aug 24 2023, 11:07 AM
libcxx/include/__locale_dir/locale_base_api/locale_guard.h
12–14

and added the comment

ldionne accepted this revision.Aug 24 2023, 11:16 AM

This LGTM.

This revision is now accepted and ready to land.Aug 24 2023, 11:16 AM
Harbormaster completed remote builds in B254681: Diff 553202.Aug 24 2023, 1:30 PM
Closed by commit rG6021c78fe55e: [libc++][Modules] locale fails to compile with clang modules when… (authored by iana). · Explain WhyAug 24 2023, 2:22 PM
This revision was automatically updated to reflect the committed changes.
iana added a commit: rG6021c78fe55e: [libc++][Modules] locale fails to compile with clang modules when….

Revision Contents

PathSize
libcxx/
include/
__locale_dir/
locale_base_api/
1 line

Diff 553200

libcxx/include/__locale_dir/locale_base_api/locale_guard.h

//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#ifndef _LIBCPP___LOCALE_LOCALE_BASE_API_LOCALE_GUARD_H #ifndef _LIBCPP___LOCALE_LOCALE_BASE_API_LOCALE_GUARD_H
#define _LIBCPP___LOCALE_LOCALE_BASE_API_LOCALE_GUARD_H #define _LIBCPP___LOCALE_LOCALE_BASE_API_LOCALE_GUARD_H
#include <__config> #include <__config>
#include <__locale>
#include <clocale> #include <clocale>
ldionneUnsubmitted
Done
ReplyInline Actions
#define _LIBCPP___LOCALE_LOCALE_BASE_API_LOCALE_GUARD_H
#include <__config>
- #include <__locale>
+ #include <__locale> // for locale_t
+ #include <clocale>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)

We still need <clocale> because we are using setlocale(...) and friends. But we should indeed add <__locale> because that is what defines locale_t when libcxx/include/__support/xlocale/__nop_locale_mgmt.h is used.

ldionne: We still need `<clocale>` because we are using `setlocale(...)` and friends. But we should…
ianaAuthorUnsubmitted
Done
ReplyInline Actions

👍 put <clocale> back

iana: 👍 put `<clocale>` back
ianaAuthorUnsubmitted
Done
ReplyInline Actions

and added the comment

iana: and added the comment
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER) #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header # pragma GCC system_header
#endif #endif
_LIBCPP_BEGIN_NAMESPACE_STD _LIBCPP_BEGIN_NAMESPACE_STD
#if !defined(_LIBCPP_LOCALE__L_EXTENSIONS) #if !defined(_LIBCPP_LOCALE__L_EXTENSIONS)
▲ Show 20 Lines • Show All 58 Lines • Show Last 20 Lines