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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
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
----------------
Mordante wrote:
> Is `Modules_` intended to be a link?
Yes, it is linked to the clang modules page: https://clang.llvm.org/docs/Modules.html. This mimics the style of https://clang.llvm.org/docs/Modules.html#where-to-learn-more-about-modules


================
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.
+
----------------
Mordante wrote:
> IMO we don't need to justify why we add a new page. WDYT?
I guess it is not bad to add it. Since it is really confusing about clang modules and C++20 Modules at first for people who are not so familiar to these topics. 


================
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.
----------------
Mordante wrote:
> Did you render the document? I would have expected double backticks.
Yeah, I had rendered it in https://overbits.herokuapp.com/rsteditor/. But it is not good. I found http://rst.aaroniles.net which looks better.


================
Comment at: clang/docs/CPlusPlus20Modules.rst:133
+  module;
+  #include <iostream>
+  #include <string>
----------------
Mordante wrote:
> Does `import <iostream>;` work? If not is that a Clang or libc++ limitation?
Yes, `import <iostream>` works here. But I don't want to introduce too many information here. And I mentioned it indirectly later in `Include translation` section.


================
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
----------------
Mordante wrote:
> Maybe explain what all steps do.
I made a simple explanation in the `#` part and I intended to explain them in more detail in the following sections. I feel it may be confusing for readers who reads at the first time?


================
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.
----------------
Mordante wrote:
> I assume `-std=c++2b` works too. Maybe rephrase the wording like this.
I sent a backport patch to 15.x: https://github.com/llvm/llvm-project/issues/56803. So it `-std=c++2b` should be fine in 15.x


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

https://reviews.llvm.org/D131062