[libcxx-commits] [PATCH] D102781: [libc++] Alphabetize header inclusions. NFCI

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/module.modulemap:548
   module __string { header "__string" export * }
+  module __threading_support { header "__threading_support" export * }
   module __tree { header "__tree" export * }
----------------
@teemperor wrote:
> I wonder if the stds_include.sh.cpp should also use the -fmodules-local-submodule-visibility flag. The default -fmodules can't really handle top modules that include textual headers. We're using the same flag in LLVM to build since D74892 (see also rG82b47b2978405f802a33b00d046e6f18ef6a47be for why)

I don't know enough modules to say whether that flag would help. However, I don't think the problem here is related to //textual// headers at all (I mean, it's not because of headers like `<cassert>`). The problem here seems to be that e.g. `<mutex>` and `<thread>` both `#include <__threading_support>`; and so the contents of header `"__threading_support"` were winding up duplicated in both `module std.mutex` and `module std.thread`. (We see random heisenbuggy duplication issues with other headers too, e.g. the partial specializations in `"__iterator/iterator_traits.h"` end up duplicated and thus ambiguous.)

What we really need is a way to "share" the common helper header `__threading_support` between `mutex` and `thread` (and so on). My impression is that the only(?) tool we have right now for doing that is to make `__threading_support` a whole module in its own right.

Personally I would be quite happy to replace all of `module.modulemap` with a single umbrella module `std` that simply includes everything; and then worry about "modularizing the STL" sometime down the road. This year in particular we seem to be having our hands full merely "headerizing" it! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102781