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

Tue Jun 6 10:26:23 PDT 2023

ldionne requested changes to this revision.
ldionne added a subscriber: var-const.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks, this looks really good but I recorded the discussion we had just now in comments.

================
Comment at: libcxx/docs/Modules.rst:58
+ * Only C++23 is tested
+ * The libc++ is not tested with modules instead of headers
+ * The module ``.cppm`` files are not installed
----------------

================
Comment at: libcxx/docs/Modules.rst:73-75
+    * Currently the tests only test with modules enabled, but not module is
+      imported. When converting tests to using modules there are still
+      failures. These are under investigation.
----------------

================
Comment at: libcxx/docs/Modules.rst:94-95
+   Once libc++'s implementation is more mature we will reach out to build
+   system vendors, with the goal that the building of the BMI files will be
+   done by the build system.
+
----------------

================
Comment at: libcxx/docs/Modules.rst:109-110
+
+The above ``build`` directory will be referred to as ``<build>`` in the other
+rest of these instructions.
+
----------------

================
Comment at: libcxx/docs/ReleaseNotes.rst:114-120
+- The lit test parameter ``enable_modules`` changed from a Boolean to an enum. The changes are
+
+  - ``False`` became ``none``. This option does not test with modules enabled.
+  - ``True`` became ``clang``. This option tests using Clang modules.
+  - ``std`` is a new optional and tests with the experimental C++23 ``std`` module.
+
+  The old values still work but will be removed in the LLVM-18.
----------------
I would move this to `Build System Changes`. Also, I think we can just change it, no need for a deprecation period here. We can consider our Lit test suite as something internal -- even though some vendors may have to make a few changes, those are easy to make and they don't affect end-users. Let's not go through more hoops than we need to.

================
Comment at: libcxx/docs/ReleaseNotes.rst:147-149
+- The lit test parameter ``enable_modules`` changed to an enum (``none``,
+  ``clang``, or ``std``) and it will no longaer support the Boolean values
+  ``True`` and ``False`` as valid input.
----------------
This can go.

================
Comment at: libcxx/test/libcxx/module_std.gen.py:46
+# See comment in the header.
+SkipDeclarations["cuchar"] = "std::mbstate_t std::size_t"
+
----------------
I would make this a list, and then use `' '.join(SkipDeclarations[header]` when you need to output it.

================
Comment at: libcxx/test/libcxx/module_std.gen.py:58-60
+SkipDeclarations[
+    "filesystem"
+] = "std::filesystem::operator== std::filesystem::operator!="
----------------
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.

================
Comment at: libcxx/test/libcxx/module_std.gen.py:66
+# TODO MODULES
+# This definition is declared in string and defined in isteam
+# This declaration should be part of string
----------------

================
Comment at: libcxx/test/libcxx/module_std.gen.py:91-93
+SkipDeclarations["type_traits"] = (
+    SkipDeclarations["type_traits"] + " std::reference_wrapper"
+)
----------------

================
Comment at: libcxx/test/libcxx/module_std.gen.py:95
+
+# Add delarations in headers.
+#
----------------

================
Comment at: libcxx/test/libcxx/module_std.gen.py:128
+
+// RUN{BLOCKLIT}: echo -n > %t.parts
+"""
----------------
This makes it a bit clearer what the file contains.

================
Comment at: libcxx/test/libcxx/module_std.gen.py:150
+        "]}' "
+        "--load=%{test-tools}/clang_tidy_checks/libcxx-tidy.plugin -- %{compile_flags} "
+        f"| sort > %t.{header}.module"
----------------

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

================
Comment at: libcxx/test/libcxx/module_std.gen.py:157
+    skip_declarations = SkipDeclarations.get(header, "")
+    if len(skip_declarations):
+        skip_declarations = (
----------------
I think you can just test for `if skip_declarations` since an empty list/string is false-y in Python.

================
Comment at: libcxx/test/libcxx/module_std.gen.py:161
+            "  key: libcpp-header-exportable-declarations.SkipDeclarations, "
+            f' value: "{skip_declarations}" '
+            "}, "
----------------

================
Comment at: libcxx/test/libcxx/module_std.gen.py:194
+        "  --load=%{test-tools}/clang_tidy_checks/libcxx-tidy.plugin "
+        "  -- %{compile_flags} "
+        f" | sort > %t.{header}.include"
----------------

================
Comment at: libcxx/test/libcxx/module_std.gen.py:223-229
+print(
+    """
+#if defined(WITH_MAIN)
+int main(int, char**) { return 0; }
+#endif
+"""
+)
----------------
I don't think this is needed at all.

================
Comment at: libcxx/test/libcxx/modules_include.sh.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
I think you need to rebase onto `main`.

================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:23
+struct clang::tidy::OptionEnumMapping<libcpp::header_exportable_declarations::FileType> {
+  static llvm::ArrayRef< std::pair<libcpp::header_exportable_declarations::FileType, llvm::StringRef>>
+  getEnumMapping() {
----------------

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

================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:134
+///
+/// TODO is this an issue in libc++?
+static std::string get_qualified_name(const clang::NamedDecl& decl) {
----------------
Per our discussion, I don't think this is an issue.

================
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;
----------------
I think we can do

```
return name.starts_with("std::__") || (name.starts_with("std::_) && std::isupper(name[6]));
```

================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.hpp:11
+
+#include <fstream>
+
----------------
I think we should IWYU properly here.

================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.hpp:16
+public:
+  header_exportable_declarations(llvm::StringRef, clang::tidy::ClangTidyContext*);
+  void registerMatchers(clang::ast_matchers::MatchFinder*) override;
----------------
IDK what's the usual convention here, but do we want this to be `explicit`?

================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:149

+  - label: "Module std C++23"
+    command: "libcxx/utils/ci/run-buildbot generic-module-std-cxx23"
----------------

================
Comment at: libcxx/utils/ci/run-buildbot:418-419
+    clean
+    # TODO MODULES Remove manual version selection.
+    export CMAKE=/opt/bin/cmake
+    generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-module-std-cxx23.cmake"
----------------
Can we do this in `buildkite-pipeline.yml` instead? Otherwise we can't run this job locally.

================
Comment at: libcxx/utils/generate_header_tests.py:76
         os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
     )
     test = pathlib.Path(os.path.join(monorepo_root, "libcxx", "test"))
----------------
This will go away upon rebasing.

================
Comment at: libcxx/utils/libcxx/test/dsl.py:446
+    std = _getSubstitution('%{cxx_std}', config)
+    if std == 'cxx2c' or std == 'cxx26':
+        std = '17' # TODO MODULES Investigate why this fails
----------------

================
Comment at: libcxx/utils/libcxx/test/dsl.py:448
+        std = '17' # TODO MODULES Investigate why this fails
+    elif std == 'cxx2b' or std == 'cxx23':
+        std = '23'
----------------

================
Comment at: libcxx/utils/libcxx/test/params.py:83-87
+    # TODO(LLVM-18) Remove this backwards compatibility.
+    fallbacks = {
+        "none": "False",
+        "clang": "True",
+    }
----------------
Not necessary.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814