[libcxx-commits] [PATCH] D151814: [libc++][modules] Adds the C++23 std module.

[Mark de Wever via Phabricator via libcxx-commits]

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

In D151814#4417987 <https://reviews.llvm.org/D151814#4417987>, @ldionne wrote:

> This pretty much looks good to me. I'd like to see it one last time after applying the comments but I think we can ship it once those comments have been applied. Thanks for all the work you did on this!

Thanks a lot for your review!



================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:60-69
+    std::string_view s = *list;
+    auto b             = s.begin();
+    auto e             = std::find(b, s.end(), ' ');
+    while (b != e) {
+      decls_.emplace(b, e);
+      if (e == s.end())
+        break;
----------------
ldionne wrote:
> Mordante wrote:
> > Mordante wrote:
> > > ldionne wrote:
> > > > We should figure out why `std::views::split(*list, ' ')` stopped working at some point and use that instead. You can loop in @var-const when you have a reproducer. Ditto below.
> > > > 
> > > > If this ends up needing a fix in `<ranges>`, I won't block the review on this comment of course but it would be nice to at least see if it's just a dumb mistake.
> > > Odd taking the code from D144994 now works again :-/ I didn't investigate why. I much rather use that code, there is a reason why I used that code in the first place.
> > I turns out that this code isn't compiled against libc++, but against the system C++ library. So that might explain the earlier failure I saw; I probably tested it on a system with an older GCC. (I noticed it when looking at the output of the most recent CI failure.)
> The question at hand is really what version of standard libraries do we support in our build tools. We've never had that question because we have very little C++ in our build tools (only the clang tidy checks) -- everything else is pretty much Python. I think what we want to do here is enforce that our build tools (that means anything we use in order to build or test the library) are written using the following requirements:
> 1. C++17 (like the rest of non-libc++ LLVM)
> 2. The compilers supported by libc++ (which are stricter than those supported by LLVM)
> 3. The standard libraries supported by the rest of non-libc++ LLVM
> 
> Concretely, this means that we basically lower all of our restrictions to be the same as non-libc++ LLVM for our build tools. I think this makes sense and should have little impact on us.
> 
> We could document that under `tools/README.md` or in our other rst documentation around contributing. Let's make it clear that this is only for build tools, though. I would also do this on a best effort basis, i.e. I wouldn't go crazy and try to add a build job that checks our minimum requirement, that seems overkill (especially given the load on our CI).
I will do that in a followup patch.


================
Comment at: libcxx/utils/libcxx/test/params.py:141-146
+            AddSubstitution(
+                "%{link_flag}",
+                lambda cfg: os.path.join(
+                    cfg.test_exec_root, "__config_module__/libc++std.a"
+                ),
+            ),
----------------
ldionne wrote:
> I think what you meant here is `AddLinkFlag(os.path.join(cfg.test_exec_root, "__config_module__/libc++std.a"))`.
> 
> Also, it would be interesting to understand why your tests worked without linking against the std module as-is.
It seems to work without it. It might be something was/is needed for the `std.compat` module. For now I removed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814