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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the reviews!



================
Comment at: libcxx/test/libcxx/module_std.gen.py:58-60
+SkipDeclarations[
+    "filesystem"
+] = "std::filesystem::operator== std::filesystem::operator!="
----------------
ldionne wrote:
> I really question what `black` is doing here with formatting. I don't think this is sensible:
> 
> ```
> SkipDeclarations[
>     "filesystem"
> ]
> ```
> 
> If that's what the tool generates, I argue the tool's not configured properly.
It's indeed black's doing. Changing this to lists improves the formatting.


================
Comment at: libcxx/test/libcxx/module_std.gen.py:91-93
+SkipDeclarations["type_traits"] = (
+    SkipDeclarations["type_traits"] + " std::reference_wrapper"
+)
----------------
ldionne wrote:
> 
I just added it directly to the list, without an append. The comment can be in the list.


================
Comment at: libcxx/test/libcxx/module_std.gen.py:156
+    # Dump the information as found in the module by using the header file(s).
+    skip_declarations = SkipDeclarations.get(header, "")
+    if len(skip_declarations):
----------------
ldionne wrote:
> 
This doesn't work, it might try to add [] to a string.
Instead I assigned the joined list to `skip_declarations`.


================
Comment at: libcxx/test/libcxx/modules_include.sh.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
ldionne wrote:
> I think you need to rebase onto `main`.
I will do that shortly before our next review, then I can move the changes to the `.cppm` files in their own 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:
> 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.


================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:180-189
+bool is_reserved_name(const std::string& name) {
+  std::size_t pos = name.find("::_");
+  if (pos == std::string::npos)
+    return false;
+
+  if (pos + 3 > name.size())
+    return false;
----------------
ldionne wrote:
> I think we can do
> 
> ```
> return name.starts_with("std::__") || (name.starts_with("std::_) && std::isupper(name[6]));
> ```
I tried that and it doesn't work. Three are elements in `std::ranges` too. I updated the comment.


================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:149
 
+  - label: "Module std C++23"
+    command: "libcxx/utils/ci/run-buildbot generic-module-std-cxx23"
----------------
ldionne wrote:
> 
As mention during review I kept std in the name since std.compat needs its own job.


================
Comment at: libcxx/utils/libcxx/test/dsl.py:446-447
+    std = _getSubstitution('%{cxx_std}', config)
+    if std == 'cxx2c' or std == 'cxx26':
+        std = '17' # TODO MODULES Investigate why this fails
+    elif std == 'cxx2b' or std == 'cxx23':
----------------
Mordante wrote:
> aaronmondal wrote:
> > Typo? Looks like this should fall back to `std = '23'` or just be `std = '26'`?
> No this is intended, hence the TODO. It seems modules and C++26 fail. I haven't gotten around to investigate this further and possibly file a bug. After this patch has landed it's easier to create a nice reproducer.
@aaronmondal actually I think I know what the issue is. CMake does not know `CMAKE_CXX_STANDARD` `26`. The current development version of CMake has this support for Clang.


================
Comment at: libcxx/utils/libcxx/test/params.py:141-145
+            AddFeature("use_module_std"),
+            AddCompileFlag("-DTEST_USE_MODULE"),
+            AddCompileFlag("-DTEST_USE_MODULE_STD"),
+            AddCompileFlag(
+                lambda cfg: "-fprebuilt-module-path="
----------------
ldionne wrote:
> I think we could simplify the CMake/Lit integration quite a bit by instead just building the BMIs from the top-level CMake invocation instead of from within Lit itself. Then we'd only need to pass the value for `-fprebuilt-module-path` to Lit and we wouldn't need `AddModule()`, the bridge changes, and also we might not need to use absolute paths everywhere.
> 
> I think this is worth investigating since it would simplify this patch quite a bit.
I tried that and it doesn't work. The module needs to be build with the flags given to lit and the standard version used. Since this can change when lit is invoked we need to do that during the test.

What I did is building a module from CMake and then rebuilt it from the lit test. That way I need 2 instead of 4 new elements in the bridge. I feel the `{modules}` directory is not too bad to have, it matches `{includes}`. Only CMake is an oddity, I hope/expect that can be removed once CMake has proper module support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814