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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added a comment.

Thanks for the comments. I will update this patch with newer revision shortly.



================
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
----------------
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.


================
Comment at: libcxx/stdmodules/std.cppm:1
+# 1 __FILE__ 1 3
+// -*- C++ -*-
----------------
philnik wrote:
> Couldn't we do
> ```
> # __LINE__ __FILE__ 1 3
> export module std;
> # __LINE__ __FILE__ 2 3
> ```
> instead?
> That should enable all the warnings inside the modules.
This is only added to be able to build this module. Clang refuses to build a module names `std` otherwise. I had tested with `__LINE__` before and that failed. I put it here since it contains the "right values" When putting it lower and with the wrong line number gives diagnostics on the wrong line.

I really want to look at a clean solution in the compiler, either something like `pragma system_header` or a compiler option. Looking into that is on my todo list.


================
Comment at: libcxx/test/std_modules/language.support/support.coroutines/coroutine.handle/coroutine.handle.capacity/operator_bool.pass.cpp:19-24
+#ifdef TEST_MODULES
+import std;
+#else
+#  include <coroutine>
+#  include <type_traits>
+#endif
----------------
philnik wrote:
> Mordante wrote:
> > ChuanqiXu wrote:
> > > philnik wrote:
> > > > Mordante wrote:
> > > > > philnik wrote:
> > > > > > Couldn't we just have a header with `import std;` in it that is always included in the C++ modules build? We have to test `#include`ing the headers and modules together anyways. That would avoid touching every single test and adding a preprocessor conditional.
> > > > > Interesting point. I think we then need 3 versions:
> > > > > - module with only an import
> > > > > - module with import and test whether works with includes
> > > > > - non-module only using includes.
> > > > > 
> > > > > But this is indeed one of the things we need to think about how we want to tackle it.
> > > > For "only an import" it would be possible to have dummy headers, which just export nothing. It's probably technically not standards-conforming, but I don't think that's a real problem.
> > > While I don't maintain libcxx, I suggest to use the current style. Since I feel it has better readability.
> > I rather not use code that's not strictly Standard conforming. Both MSVC STL and libstdc++ use our test suite, so violating the Standard may work for us, but it may break them.
> I'm pretty sure that's exactly as technically-non-conforming as checking for `_LIBCPP_VERSION`. No implementation will ever have a problem with it.
I see checking for `_LIBCPP_VERSION` a bit different. I will add this point to the list of items to discuss. For now I'll keep this solution since it's easier to "convert" a batch of tests and test whether they work with modules. (There might be other ways for testing that, but I started to use this and it works).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144994