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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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;
----------------
Mordante wrote:
> Mordante wrote:
> > 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.
> I've created D152377.
I think that's referencing the wrong patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814