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

Wed Aug 2 20:24:46 PDT 2023

philnik added a comment.

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.

>> w.r.t. back-porting this it's a definite no from me, since this is an improvement of the library. Just like we shouldn't back-port other performance improvements without a really good reason. This feature is also still experimental. There is always a next release in a few months.
>
> I think 8.x compilation speedup and 10.x size reduction may be a good reason.

I don*t think so. We have these kinds of speedups all the time. I have like a dozen patches locally that reach these kinds of speedups (some even 300x) for algorithms, but we shouldn't back-port them because they are improvements; they aren't fixing a regression.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156907