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

[Aaron Siddhartha Mondal via Phabricator via libcxx-commits]

aaronmondal added inline comments.


================
Comment at: libcxx/CMakeLists.txt:108
 option(LIBCXX_ENABLE_CLANG_TIDY "Whether to compile and run clang-tidy checks" OFF)
+option(LIBCXX_ENABLE_STD_MODULE
+   "Whether to enable the building the C++23 `std` module. This feature is
----------------
For future compatiblity maybe this should be `LIBCXX_ENABLE_STD_MODULES` (with `S`) or `LIBCXX_ENABLE_MODULES` instead? At the moment only `std` is supported, but when `std.compat` is added later the current flag name could be misleading.

Or should there be different flags for `std` and `std.compat`? IMO they should both be covered by the same flag.


================
Comment at: libcxx/docs/Modules.rst:65
+      `bug report <https://github.com/llvm/llvm-project/issues/61465>`__.
+
+Blockers
----------------
As a safeguard we might want to add the note that the BMIs shouldn't be distributed at the moment. AFAIK BMIs still contain absolute paths to their build environment. I.e. if you build at `/my/custom/build/machine` this path will be printed if you run `strings` on the BMIs. This could leak information from a distributors build environment to the BMI.


================
Comment at: libcxx/modules/CMakeLists.txt.in:62
+	-Wno-reserved-user-defined-literal
+	#-Werror
+    @LIBCXX_COMPILE_FLAGS@
----------------
I believe only https://github.com/llvm/llvm-project/issues/62844 breaks `-Werror` when building with C++23. Alternatively, `-Wno-deprecated-declarations` might also allow use of `-Werror`.


================
Comment at: libcxx/utils/ci/run-buildbot:200
         --diff \
-        --extensions ',h,hpp,c,cpp,inc,ipp' HEAD~1 \
-        -- $(find libcxx/{benchmarks,include,src}/ -type f | grep -vf libcxx/utils/data/ignore_format.txt) \
+        --extensions ',h,hpp,c,cpp,cppm,inc,ipp' HEAD~1 \
+        -- $(find libcxx/{benchmarks,include,modules,src}/ -type f | grep -vf libcxx/utils/data/ignore_format.txt) \
----------------
build2 encourages the extensions `mxx` and `mpp`. Adding those here might save them a downstream patch :D


================
Comment at: libcxx/utils/generate_header_tests.py:85
+    produce(test.joinpath("libcxx/module_std.sh.cpp"), libcxx.test.header_information.variables)
     produce(test.joinpath("libcxx/nasty_macros.compile.pass.cpp"), libcxx.test.header_information.variables)
     produce(test.joinpath("libcxx/no_assert_include.compile.pass.cpp"), libcxx.test.header_information.variables)
----------------
Unrelated, to this patch. Some styleguides consider `nasty` as "unlikely profanity". There might be a better way to name this.


================
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':
----------------
Typo? Looks like this should fall back to `std = '23'` or just be `std = '26'`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814