[libcxx-commits] [PATCH] D144322: [libc++][Modules] Make top level modules for all C++ headers with OS/clang versions

[Ian Anderson via Phabricator via libcxx-commits]

iana added inline comments.


================
Comment at: libcxx/include/module.modulemap.in:12
     module ctype_h {
-      header "ctype.h"
-      export *
+      export std_ctype_h
     }
----------------
vsapsai wrote:
> iana wrote:
> > iana wrote:
> > > I'm almost positive all these export lines don't do anything besides inform the reader where the header has moved. This seems to be the style in Apple's module maps when headers move, but I'm not sure if there's any practical reason for doing it. Keeping the modules around would make it so that if anyone actually did `@import std.depr.ctype_h;` then that wouldn't result in an error. They'd probably still get ctype.h via something else in `std` including that header, but it wouldn't be guaranteed.
> > > 
> > > We could make underscored headers for all of these that just include the regular version to force `std` to transitively include all of the new top level modules. We could also make a single "dependencies" header that includes all of them, and add that to the `std` module. Or maybe the way it is is fine?
> > I did some testing, and these `export` lines do absolutely nothing. If we want to make them actually export the new top level modules, we need cover headers like _ctype.h that includes ctype.h purely for module re-export purposes. Probably with a `__building_module` guard too. Or, just get rid of these submodules completely. I think it's only a problem if there are some clients out there that `import std.depr.ctype_h`, but that seems pretty unlikely to me.
> Will `@import std.compat.cstdio;` do anything useful after this change? Haven't checked with the actual code.
Nope, it does nothing at all. It looks like empty submodules that export other submodules in the same module works, but empty submodules that export a different top level module doesn't do anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322