[PATCH] D131062: [docs] Add "C++20 Modules"

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

Thanks a lot for working on this!

I wonder whether it would be better to have some of the more technical information in a separate file and let this file mainly have an introduction to modules in general and how to get started using them in Clang.

I haven't tested the steps yet, but I intend to see whether the examples work for me.

I haven't validated whether the module explanation is correct; I'm sure there are other reviewers who have a deeper knowledge of modules.



================
Comment at: clang/docs/CPlusPlus20Modules.rst:11
+
+Modules have a lot of meanings. For the users of clang compiler, modules may
+refer to `Objective-C Modules`, `Clang C++ Modules` (or `Clang Header Modules`,
----------------
Clang is capitalized in its documentation.


================
Comment at: clang/docs/CPlusPlus20Modules.rst:18
+
+There is already a detailed document about clang modules Modules_, it
+should be helpful to read Modules_ if you want to know more about the general
----------------
Is `Modules_` intended to be a link?


================
Comment at: clang/docs/CPlusPlus20Modules.rst:20
+should be helpful to read Modules_ if you want to know more about the general
+idea of modules. But due to the C++20 modules having very different semantics, it
+might be more friendly for users who care about C++20 modules only to create a
----------------



================
Comment at: clang/docs/CPlusPlus20Modules.rst:22
+might be more friendly for users who care about C++20 modules only to create a
+new page.
+
----------------
IMO we don't need to justify why we add a new page. WDYT?


================
Comment at: clang/docs/CPlusPlus20Modules.rst:35
+tutorial. The document would only introduce concepts about the the
+structure and building of the project.
+
----------------
How about mentioning the state of modules in Clang? In your GitHub repository you mention some limitations. (A link to a status page could be an alternative.)


================
Comment at: clang/docs/CPlusPlus20Modules.rst:56
+
+A module consists of one or multiple module units. A module unit is a special
+translation unit. Every module unit should have a module declaration. The syntax
----------------



================
Comment at: clang/docs/CPlusPlus20Modules.rst:79
+A primary module interface unit is a module unit whose module declaration is
+`export module module_name;`. The `module_name` here denotes the name of the
+module. A module should have one and only primary module interface unit.
----------------
Did you render the document? I would have expected double backticks.


================
Comment at: clang/docs/CPlusPlus20Modules.rst:117
+
+Here is a hello world example to show how to use modules.
+
----------------
Maybe make this hello world example a bit simpler by not using partitions and have a separate example with partitions. This example gives the impression a simple "hello modular world" is a lot harder to achieve than a normal "hello world".


================
Comment at: clang/docs/CPlusPlus20Modules.rst:133
+  module;
+  #include <iostream>
+  #include <string>
----------------
Does `import <iostream>;` work? If not is that a Clang or libc++ limitation?


================
Comment at: clang/docs/CPlusPlus20Modules.rst:164
+  # Precompiling the module
+  clang++ -std=c++20 interface_part.cppm --precompile -o M-interface_part.pcm
+  clang++ -std=c++20 impl_part.cppm --precompile -fprebuilt-module-path=. -o M-impl_part.pcm
----------------
Maybe explain what all steps do.


================
Comment at: clang/docs/CPlusPlus20Modules.rst:178
+
+We would explain the options in the following sections.
+
----------------



================
Comment at: clang/docs/CPlusPlus20Modules.rst:183
+
+Currently, C++20 Modules is enabled automatically if the language standard is `-std=c++20`.
+Sadly, we can't enable C++20 Modules now with lower language standard versions like coroutines by `-fcoroutines-ts` due to some implementation problems.
----------------
I assume `-std=c++2b` works too. Maybe rephrase the wording like this.


================
Comment at: clang/docs/CPlusPlus20Modules.rst:184
+Currently, C++20 Modules is enabled automatically if the language standard is `-std=c++20`.
+Sadly, we can't enable C++20 Modules now with lower language standard versions like coroutines by `-fcoroutines-ts` due to some implementation problems.
+The `-fmodules-ts` option is deprecated and is planned to be removed.
----------------
I would remove this sentence, it's not too common to backport language features.


================
Comment at: clang/docs/CPlusPlus20Modules.rst:321
+
+If the user don't want to follow the consistency requirement due to some reasons (e.g., distributing module files),
+the user could try to use `-Xclang -fmodules-embed-all-files` when producing module files. For example:
----------------



================
Comment at: clang/docs/CPlusPlus20Modules.rst:539
+
+Since there is already one module maps in the source of libcxx.
+
----------------
Maybe provide a link to this module map. (FYI the libc++ module map is generated by CMake using a `.in` file.)


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

https://reviews.llvm.org/D131062