Skip to content

[libc++] First attempt to regroup a few modules in the modulemap #98214

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 30, 2024
Merged

[libc++] First attempt to regroup a few modules in the modulemap #98214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 20 additions & 46 deletions libcxx/include/module.modulemap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,15 @@ module std_stdexcept [system] {
header "stdexcept"
export *
}
module std_stop_token {
module std_stop_token [system] {
header "stop_token"
private header "__stop_token/atomic_unique_lock.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these still be submodules?

Copy link
Member Author

@ldionne ldionne Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know? Is there a reason why we should prefer making it a submodule rather than listing private headers like this? Genuine question, I'm not an expert on Clang modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes generally every header should be in its own submodule. Otherwise including stop_token when building with modules will implicitly include all of these headers as well, even if stop_token itself doesn't include them.

Copy link
Member Author

Choose a reason for hiding this comment

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

But <stop_token> does need these headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well yes. The stop_token implementation headers are a bit unique in that stop_token itself directly includes all of them, and none of the other headers do. Something more typical is like __type_traits headers. You can't do like this.

module std_type_traits [system] {
  header "type_traits"
  private header "__type_traits/A.h"
  export *
}
  1. If say algorithm includes __type_traits/A.h it will also get type_traits and all of the other headers declared at the same level.
  2. algorithm can't include __type_traits/A.h because it's private.

So while this is fine for stop_token, it's going to unravel fast when you try to do some of the more significant ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like a surprising best practice. Aren't we trying to stop building so many small modules in the first place? The way I understand it:

  • stop_token is a module, and it has both public APIs and private APIs, as denoted by the public/private headers declared in that module
  • type_traits is a module, but it has almost exclusively public headers because __type_traits/xxxxx.h are meant to be included directly from other parts of the code.

Either way -- we don't currently expect other parts of the code to include implementation details of <stop_token>. If they did, I would argue for moving these parts (e.g. intrusive_shared_ptr) to another module that contains generally-useful utilities, or to memory, or something. But obviously <stop_token> should stay a leaf module whatever happens, and in that sense I think it makes most sense to define it the way it is currently defined (with private headers).

Copy link
Contributor

@ian-twilightcoder ian-twilightcoder Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Submodules only control visibility. If you make a submodule for all of the __stop_token headers, it'll still build all of them into a single pcm. It only controls what's visible when something includes __stop_token/atomic_unique_lock.h. i.e. if you have this.

module std_stop_token [system] {
  header "stop_token"
  module atomic_unique_lock { private header "__stop_token/atomic_unique_lock.h" }
  ...
}

Then std_stop_token builds the same as it does the way you have it right now and still cuts down on all the module builds/pcms. The difference is that if something includes __stop_token/atomic_unique_lock.h, then it behaves like non-modular C and you only get __stop_token/atomic_unique_lock.h. But the way you have it, if you build with modules then you also get stop_token and all of the other __stop_token headers, which is pretty unintuitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, thanks for the clarification. This is clearly relevant for how to setup e.g. type_traits. However, for stop_token, nobody outside of the stop_token module can include __stop_token/atomic_unique_lock.h, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as you keep it a private header, yes that's right.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I believe the original mega module had submodules for all of the implementation headers like I'm suggesting btw)

private header "__stop_token/intrusive_list_view.h"
private header "__stop_token/intrusive_shared_ptr.h"
private header "__stop_token/stop_callback.h"
private header "__stop_token/stop_source.h"
private header "__stop_token/stop_state.h"
private header "__stop_token/stop_token.h"
export *
}
module std_streambuf [system] {
Expand Down Expand Up @@ -1584,41 +1591,25 @@ module std_private_numeric_transform_exclusive_scan [system] { header "__numeric
module std_private_numeric_transform_inclusive_scan [system] { header "__numeric/transform_inclusive_scan.h" }
module std_private_numeric_transform_reduce [system] { header "__numeric/transform_reduce.h" }

module std_private_pstl_backend [system] {
module std_private_pstl [system] {
header "__pstl/backend.h"
export *
}
module std_private_pstl_backend_fwd [system] {
header "__pstl/backend_fwd.h"
export *
}
module std_private_pstl_backends_default [system] {
header "__pstl/backends/default.h"
export *
}
module std_private_pstl_backends_libdispatch [system] {
header "__pstl/backends/libdispatch.h"
export *
}
module std_private_pstl_backends_serial [system] {
header "__pstl/backends/serial.h"
export *
}
module std_private_pstl_backends_std_thread [system] {
header "__pstl/backends/std_thread.h"
export *
header "__pstl/cpu_algos/any_of.h"
header "__pstl/cpu_algos/cpu_traits.h"
header "__pstl/cpu_algos/fill.h"
header "__pstl/cpu_algos/find_if.h"
header "__pstl/cpu_algos/for_each.h"
header "__pstl/cpu_algos/merge.h"
header "__pstl/cpu_algos/stable_sort.h"
header "__pstl/cpu_algos/transform.h"
header "__pstl/cpu_algos/transform_reduce.h"
header "__pstl/dispatch.h"
header "__pstl/handle_exception.h"
}
module std_private_pstl_cpu_algos_any_of [system] { header "__pstl/cpu_algos/any_of.h" }
module std_private_pstl_cpu_algos_cpu_traits [system] { header "__pstl/cpu_algos/cpu_traits.h" }
module std_private_pstl_cpu_algos_fill [system] { header "__pstl/cpu_algos/fill.h" }
module std_private_pstl_cpu_algos_find_if [system] { header "__pstl/cpu_algos/find_if.h" }
module std_private_pstl_cpu_algos_for_each [system] { header "__pstl/cpu_algos/for_each.h" }
module std_private_pstl_cpu_algos_merge [system] { header "__pstl/cpu_algos/merge.h" }
module std_private_pstl_cpu_algos_stable_sort [system] { header "__pstl/cpu_algos/stable_sort.h" }
module std_private_pstl_cpu_algos_transform [system] { header "__pstl/cpu_algos/transform.h" }
module std_private_pstl_cpu_algos_transform_reduce [system] { header "__pstl/cpu_algos/transform_reduce.h" }
module std_private_pstl_dispatch [system] { header "__pstl/dispatch.h" }
module std_private_pstl_handle_exception [system] { header "__pstl/handle_exception.h" }

module std_private_queue_fwd [system] { header "__fwd/queue.h" }

Expand Down Expand Up @@ -1773,23 +1764,6 @@ module std_private_span_span_fwd [system] { header "__fwd/span.h" }

module std_private_stack_fwd [system] { header "__fwd/stack.h" }

module std_private_stop_token_atomic_unique_lock [system] { header "__stop_token/atomic_unique_lock.h" }
module std_private_stop_token_intrusive_list_view [system] { header "__stop_token/intrusive_list_view.h" }
module std_private_stop_token_intrusive_shared_ptr [system] { header "__stop_token/intrusive_shared_ptr.h" }
module std_private_stop_token_stop_callback [system] { header "__stop_token/stop_callback.h" }
module std_private_stop_token_stop_source [system] {
header "__stop_token/stop_source.h"
export *
}
module std_private_stop_token_stop_state [system] {
header "__stop_token/stop_state.h"
export *
}
module std_private_stop_token_stop_token [system] {
header "__stop_token/stop_token.h"
export *
}

module std_private_string_char_traits [system] {
header "__string/char_traits.h"
export *
Expand Down
7 changes: 3 additions & 4 deletions libcxx/test/libcxx/thread/thread.stoptoken/atomic_unique_lock.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// UNSUPPORTED: no-threads

// XFAIL: availability-synchronization_library-missing

// UNSUPPORTED: no-threads
// UNSUPPORTED: c++03, c++11, c++14, c++17
// XFAIL: availability-synchronization_library-missing
// ADDITIONAL_COMPILE_FLAGS: -Wno-private-header

#include <__stop_token/atomic_unique_lock.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead just add -Wno-private-header to all libcxx/test/libcxx tests? I think ideally we wouldn't export library details at all other than by including detail headers.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, but I think I'd rather add it on a per test basis to avoid making it too easy to include detail headers unknowingly.

#include <atomic>
Expand Down
1 change: 1 addition & 0 deletions libcxx/test/libcxx/thread/thread.stoptoken/intrusive_list_view.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
//

// UNSUPPORTED: c++03, c++11, c++14, c++17
// ADDITIONAL_COMPILE_FLAGS: -Wno-private-header

#include <__stop_token/intrusive_list_view.h>
#include <cassert>
Expand Down
1 change: 1 addition & 0 deletions libcxx/test/libcxx/thread/thread.stoptoken/intrusive_shared_ptr.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
//

// UNSUPPORTED: c++03, c++11, c++14, c++17
// ADDITIONAL_COMPILE_FLAGS: -Wno-private-header

#include <__stop_token/intrusive_shared_ptr.h>
#include <atomic>
Expand Down
Loading