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

[Ian Anderson via Phabricator via libcxx-commits]

iana added a comment.

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++.



================
Comment at: libcxx/include/__std:10
+
+#if !__building_module(std)
+#error "Do not include this header directly, include individual headers instead"
----------------
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.)


================
Comment at: libcxx/include/__std:80
+#include <strstream>
+#include <system_error>
+#include <thread>
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156177