[libcxx-commits] [PATCH] D156177: [libc++][Modules] Recreate the top level `std` clang module

Tue Jul 25 11:18:14 PDT 2023

Mordante added a comment.

In D156177#4532637 <https://reviews.llvm.org/D156177#4532637>, @iana wrote:

> In D156177#4532451 <https://reviews.llvm.org/D156177#4532451>, @Mordante wrote:
>
>> In D156177#4532349 <https://reviews.llvm.org/D156177#4532349>, @aprantl wrote:
>>
>>>> ! In D156177#4532335 <https://reviews.llvm.org/D156177#4532335>, @Mordante wrote:
>>>>  I'm not entirely fond of this approach.
>>>
>>> Would you mind clarifying what your concerns are?
>>
>> My main concern is shipping a header to all our users that includes everything without me understanding why. Especially since that was not needed before the changes to modules. Hence my question why this is needed by LLDB.
>>
>> Smaller concerns, which are fixable
>>
>> - Something else we need to maintain. We don't often add new top-level headers and I don't see tooling to make sure we don't forget to add it.
>> - Using a module named `std` which is a C++23 feature
>
> The goal is to have a single clang module that lldb can import to get all of libc++. And maybe for anyone else that's currently importing the shipping `std` clang module, although they're probably better off importing the individual modules they use like `std_algorithm`. clang modules require header files, they can't do anything without them. So for the `std` clang module to provide everything from libc++, it needs a header that includes everything from libc++.

Does this mean the current module changes may be a breaking change for clang module users?
I know this is used at Google.

How did this work before your change? IIRC we didn't have a header to include at that time.
I really would like to understand why we need a header now, when it could be done without a header before.

> We're shipping a new header that includes everything, but if anyone includes it they'll get an error. (That's what the `__building_module` check does - it makes the header error for everything _except_ doing `import std`.)

Thanks for the info.

> clang modules are getting to be kind of a pain to support.

+1

> I'm in favor of scripting that pain away, but @ldionne felt that I was just exchanging manual header/module map editing pain for debugging Python pain.

Unlike D155141 <https://reviews.llvm.org/D155141> a script to generate this header should be trivial. The file `libcxx/utils/libcxx/test/header_information.py` already has the logic to get all `public_headers` in libc++. This script is used for our testing so this script will get updated when the rules for our public headers change. I feel here scripting will make things less error prone. We already have a CMake step to build generated files. So I think this would be a natural fit to add there.

> The module name `std` is what the current (llvm 16) clang module name is for libc++. So that part isn't a change, it's just restoring the behavior from a few weeks ago.

I see, then we can't change that.

================
Comment at: libcxx/include/__std:10
+
+#if !__building_module(std)
+#error "Do not include this header directly, include individual headers instead"
----------------
iana wrote:
> Mordante wrote:
> > Mordante wrote:
> > > Where does this `__building_module` come from? I'm not happy with this line since will confuse people. C++23 adds the named module `std`. This has nothing to do with that module.
> > I think this file name can benefit from something longer. This include is only needed in a very special case.
> `__building_module` is a clang builtin. Happy to call the header anything, was just guessing on what the convention might be. Do you like `__std_clang_module`? Should it have .h? (I don't know why __std_mbstate_t.h has .h and __locale doesn't.)
I think that name would help to avoid the confusion. Adding a short comment to this header would help too.
Typically we only use `.h` for C headers. There might be some inconsistencies.

================
Comment at: libcxx/include/__std:80
+#include <strstream>
+#include <system_error>
+#include <thread>
----------------
iana wrote:
> Mordante wrote:
> > When somebody implements the header `text_encoding` will there be a CI tests that fails?
> > 
> > We have the checklist, which you updated, but in general we like to have a red CI when somebody forgets it.
> No, no test right now. If we're going to write a test to check what the contents of this header should be, why not just make that code write the header too?
I'm in favor of a script, I've added more details below.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156177/new/

https://reviews.llvm.org/D156177