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

[Raphael Isemann via Phabricator via libcxx-commits]

teemperor 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 * }
----------------
Quuxplusone wrote:
> @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! :)
Sorry, I think I should have made clear that I meant 'headers that are not in a module' when I said 'textual' (as those headers behave similar to the actual textual headers declared in a modulemap).

And yes, having all the headers (even helpers) in their own submodule would probably help a lot with the bugs caused by the duplication. I'm not familiar enough with libc++ to say how much work that would be. Just commenting as I pushed for that specific test and I don't just want to leave it to others to keep it green :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102781