[libcxx-commits] [PATCH] D144994: [Draft][libc++][modules] Adds std module.

Fri Mar 10 10:16:27 PST 2023

Mordante added a comment.

In D144994#4176269 <https://reviews.llvm.org/D144994#4176269>, @EricWF wrote:

> Cool deal. Thanks for working on this.

Thanks!

In D144994#4173764 <https://reviews.llvm.org/D144994#4173764>, @ChuanqiXu wrote:

>> One of the issues in libc++ is that vendors can decide to disable filesystem header. Since parts of that library are in the dylib the std module can't provide what's in filesystem. Does your method also have that feature?
>
> I disabled the filesystem header directly in the downstream... so I don't have a lot experience in this.

Thanks for the information!

>> I think we can discuss that and see whether it's a blocker for landing the patch or not. For now I still have issues with ranges (which I have not started to investigate). I have been experimenting with running the test-suite for the modular build. So I will have some more information on that later today, probably on Discord.
>
> Yeah, it is always good to have more experience. And I didn't expect that we can convert all headers and pass all the tests in one shot. It is good enough for me to have a std module which contains a lot of headers and can pass a lot of tests.

I also didn't expect that, but we need to look at when we're comfortable with shipping something. Since libc++ is a system library on some platforms we need to consider that carefully. Obviously there are other non-official ways to ship things too.

It seems quite some tests fail due to using `size_t` instead of `std::size_t` and similar C-types. I'm working on patches to use the qualified name instead.

================
Comment at: libcxx/stdmodules/std-format.cppm:12
+// TODO This file is two-fold
+// - look at how we want to integrate a synopsis
+// - test whether the experimental library works properly
----------------
philnik wrote:
> ldionne wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > aaronmondal wrote:
> > > > > philnik wrote:
> > > > > > My stance is still that we should just not have it/remove it. Having notes on the papers/extra status pages for large papers is way more reliable than looking through the synopsis, but I guess nobody agrees with me here.
> > > > > +1 from me.
> > > > > 
> > > > > Personally, I find the style you suggested in https://reviews.llvm.org/D135507 very readable.
> > > > > 
> > > > > Potentially with links to the corresponding sections/papers even. A broken/outdated link is still a lot better than a potentially ever so slightly outdated docstring.
> > > > > My stance is still that we should just not have it/remove it. Having notes on the papers/extra status pages for large papers is way more reliable than looking through the synopsis, but I guess nobody agrees with me here.
> > > > 
> > > > I disagree that nobody agrees with you. I'm also not fond of the current synopsis, since it's quite often out of date. I discussed that a few weeks ago privately with Louis and when we discussed modules he thought these files would be a better place for the synopsis. That's why I added it here, however for now I only do that for one header, where I exactly know the implementation status. For other headers this will be more effort.
> > > Good to know.
> > Well, if nobody like synopsis in headers, we can also remove them :-). Let's chat about that.
> > 
> > To be clear, you are not opposed to keeping the synopsis comment in test files that describes what we're testing, right? I think those are definitely useful and we should not stop those.
> > 
> > Pinging @var-const @huixie90 for thoughts. If folks agree, I don't want to impose it on people.
> > 
> > FWIW I do agree that storing those in the `.cppm` files results in something less readable than I thought it would.
> > Well, if nobody like synopsis in headers, we can also remove them :-). Let's chat about that.
> > 
> > To be clear, you are not opposed to keeping the synopsis comment in test files that describes what we're testing, right? I think those are definitely useful and we should not stop those.
> I'm not sure they are always super useful, but they are definitely in enough cases that it would probably be a bad idea to remove them. They are also much easier to maintain and less error-prone.
> 
> Well, if nobody like synopsis in headers, we can also remove them :-). Let's chat about that.
> 
> To be clear, you are not opposed to keeping the synopsis comment in test files that describes what we're testing, right? I think those are definitely useful and we should not stop those.
> 
> Pinging @var-const @huixie90 for thoughts. If folks agree, I don't want to impose it on people.
> 
> FWIW I do agree that storing those in the `.cppm` files results in something less readable than I thought it would.

+1 for discussing the synopsis. But let's take it to Discord or a libc++ meeting.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144994