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

[Ian Anderson via Phabricator via libcxx-commits]

iana added inline comments.


================
Comment at: libcxx/docs/Contributing.rst:50
 
+  - Did you add it to ``include/__std``?
   - Did you add it to ``include/module.modulemap.in``?
----------------
Mordante wrote:
> Since we're going to add a script, so this can be removed.
Discussed in Discord, I'll make a script in a separate review later today, I'll remove this comment then.


================
Comment at: libcxx/include/CMakeLists.txt:655
   __split_buffer
+  __std
   __std_mbstate_t.h
----------------
Mordante wrote:
> Is there a reason we need to ship this header to all users of libc++?
We have to have a header to build the module, so unfortunately it is necessary. However, the `__building_module` should give an error to anyone who tries to `#include` it.


================
Comment at: libcxx/include/__std:49
+#include <list>
+#include <locale>
+#include <map>
----------------
Mordante wrote:
> Please check which headers can be included in the libc++ build. Including this header will fail when locales are disabled.
> `libcxx/utils/libcxx/test/header_information.py` has this information.
Oooh yes, excellent point, sorry. I'd better make a test that `@import`s this in ObjC++ to make sure it works with all the parts disabled (even after the generated version)


================
Comment at: libcxx/include/__std:10
+
+#if !__building_module(std)
+#error "Do not include this header directly, include individual headers instead"
----------------
Mordante wrote:
> 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.
Sounds good, I'll use `__std_clang_module` and put a comment in it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156177