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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for the review!



================
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
----------------
aaronmondal wrote:
> 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.
They indeed should be the same flag. The reason why this is an option is due to the additional version requirements of CMake, ninja, and Clang. These are the same for `std` and `std.compat`. The intention is to remove this option at some point in the future.

I should add a TODO MODULES to make that clear.


================
Comment at: libcxx/docs/Modules.rst:65
+      `bug report <https://github.com/llvm/llvm-project/issues/61465>`__.
+
+Blockers
----------------
aaronmondal wrote:
> 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.
I'm not sure that is needed. I feel there are already quite some warnings that BMIs are not portable. At least on Debian in the past, debug packages "leaked" the same information, which didn't seem to be an issue.

Do you have reasons why this is a real issue?


================
Comment at: libcxx/modules/CMakeLists.txt.in:62
+	-Wno-reserved-user-defined-literal
+	#-Werror
+    @LIBCXX_COMPILE_FLAGS@
----------------
aaronmondal wrote:
> 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`.
`-Werror` should be removed, that's left from testing. Nice catch!

I really don't want this since users could add other compiler flags that will cause warnings to be issued.


================
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) \
----------------
aaronmondal wrote:
> build2 encourages the extensions `mxx` and `mpp`. Adding those here might save them a downstream patch :D
This only tests formatting extensions used in libc++. For example I recently removed `hxx` and `cxx` since we don't use them. The main reason to specify these manually is that we want to format files without any extension. For example `string`.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814