[libcxx-commits] [PATCH] D156907: [libc++][modules] Removes the module partitions.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D156907#4556195 <https://reviews.llvm.org/D156907#4556195>, @philnik wrote:

> In D156907#4556159 <https://reviews.llvm.org/D156907#4556159>, @ChuanqiXu wrote:
>
>> In D156907#4556138 <https://reviews.llvm.org/D156907#4556138>, @philnik wrote:
>>
>>> I'm a bit concerned about the maintainability of this. AFAICT this change makes it basically impossible for tooling to parse the modules correctly. Would it be possible to have one partition that includes everything and have other modules do the actual exports (or something similar)? That should allow tooling to parse everything correctly and we don't have the problem that we have to parse a lot of code multiple times. Another option would be to just have a single huge file. I haven't looked at everything, but the total size would probably be < 5k lines, which wouldn't even make it the largest file we have in the library.
>>
>> While the overhead to introduce another partition should be small, I don't understand what's the problem for tools to parse the modules correctly now?
>
> The problem is that the files aren't self-contained, so a tool parsing only a single file doesn't know that other stuff happens to already be included. That's a huge problem for IDEs.

I think it is an issue, but not a huge issue. I added a README explaining the design. But basically the std module need to look like

  module;
  
  #include <all_libc++_public_headers>
  
  export module std;
  
  export namespace std {
  
    using std::all_public_named_declarations;
  
  }

The split in the `#include` and `using` is the issue. In the future it might be possible to consolidate it again like

  export module std;
  
  #include <libc++_public_header_1>
  export namespace std {
    using std::all_public_named_declarations_of_header_1;
  }
  
  #include <libc++_public_header_2>
  export namespace std {
    using std::all_public_named_declarations_of_header_2;
  }

But include after export is not allowed.

In D156907#4558564 <https://reviews.llvm.org/D156907#4558564>, @aaronmondal wrote:

> I like this change a lot. Seems like its strictly better than the previous variant. Are there any "downsides" to this change? I guess just the the single module file takes longer to build since its one gigantic file?
>
> "Downstream-wise", the Bazel implementation will be simple to get working with this change and the improved buildtime and artifact sizes far outweigh the slight inconvenience of changing buildfiles.

It also removes the special case for `__new` for you :-)

> No strong opinions regarding backporting. I'd expect users of such experimental technologies to live at head anyways 😅

Agreed.

In D156907#4559552 <https://reviews.llvm.org/D156907#4559552>, @ChuanqiXu wrote:

> Oh, wait a minute. I just realized that we didn't ship module files, right? So that the users who get libc++ by things like `apt/yum install` wouldn't see the std module. Then the people who can access the std module must be the people who are able to compile it from source. If all of these things are true, it is indeed not necessary to backport this since nearly no release users will get benefits from it.

Yes modules are not installed yet since they are still experimental and I didn't have enough time to work on this part before LLVM-17.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156907