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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Adds some notes for reviewing.



================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:61
+  // TODO(LLVM-17) Remove clang 15 work-around.
+#if defined(__clang_major__) && __clang_major__ < 16
+  if (list) {
----------------
The code is not used for Clang < 17 since these compilers lack module support. However there is a working implementation for these versions of Clang. The original code fails in https://buildkite.com/llvm-project/libcxx-ci/builds/25748. Since we'll drop LLVM 15 support rather soon, I prefer this work-around for the short term.

Looking at the build log, it seems to be an issue with concepts:

```
clang_tidy_checks/header_exportable_declarations.cpp:9:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyCheck.h:12:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyDiagnosticConsumer.h:12:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyOptions.h:12:

In file included from /usr/lib/llvm-17/include/llvm/ADT/IntrusiveRefCntPtr.h:66:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/memory:66:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tempbuf.h:61:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_construct.h:61:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_iterator_base_types.h:71:

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/iterator_concepts.h:999:13: error: no matching function for call to '__begin'

        = decltype(ranges::__cust_access::__begin(std::declval<_Tp&>()));

                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/ranges_base.h:513:5: note: in instantiation of template type alias '__range_iter_t' requested here

    using iterator_t = std::__detail::__range_iter_t<_Tp>;

    ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/ranges_util.h:131:36: note: in instantiation of template type alias 'iterator_t' requested here

      requires contiguous_iterator<iterator_t<_Derived>>

                                   ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1134:29: note: in instantiation of template class 'std::ranges::view_interface<std::ranges::ref_view<llvm::StringRef>>' requested here

    class ref_view : public view_interface<ref_view<_Range>>

                            ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:38: note: in instantiation of template class 'std::ranges::ref_view<llvm::StringRef>' requested here

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                            ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:38: note: in instantiation of requirement here

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:27: note: (skipping 8 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                 ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3717:6: note: while substituting template arguments into constraint expression here

          = requires { split_view(std::declval<_Range>(), std::declval<_Pattern>()); };

            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3723:11: note: while checking the satisfaction of concept '__can_split_view<llvm::StringRef &, char>' requested here

        requires __detail::__can_split_view<_Range, _Pattern>

                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3723:11: note: while substituting template arguments into constraint expression here

        requires __detail::__can_split_view<_Range, _Pattern>

                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814