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

Sun Aug 7 20:19:20 PDT 2022

ChuanqiXu added a reviewer: h-vetinari.
ChuanqiXu marked 45 inline comments as done.
ChuanqiXu added a comment.

In D131062#3704017 <https://reviews.llvm.org/D131062#3704017>, @h-vetinari wrote:

>> It would be greatly welcome for such comments!
>
> OK, here goes. Sorry for the large volume of comments. In addition to typos and stylistic improvements, I've had a few questions where the content wasn't clear to me (but note I'm not experienced with modules at all, so this may be obvious to others). I've also added a few line breaks where Phab was awkwardly overflowing. The position of the linebreaks is obviously irrelevant, but I thought it might help further review.

Many thanks for the though review! It is very helpful.

> I've had a few questions where the content wasn't clear to me (but note I'm not experienced with modules at all, so this may be obvious to others)

This is welcome. In fact, it is a failure if the document requires the reader to have experienced with modules.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:15-16
+share a big part of codes. But from the perspective of users, their semantics and
+command line interfaces are very different. So it should be helpful for the users
+to introduce how to use C++20 modules.
+
----------------
h-vetinari wrote:
> Not 100% what the intention of the last sentence is - I presume it sets the goal for this document?
The intention was to describe the motivation of the document, but we have stated it again in the next paragraph. So the suggestion looks fine.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:20-21
+should be helpful to read `Clang modules <Modules.html>`_ if you want to know
+more about the general idea of modules. 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 new page.
----------------
h-vetinari wrote:
> > 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 new page.
> 
> Isn't "C++20 modules" what this page intends to do? Which new page are we talking about then?
The `new page` here refers to the document. The intention here is that the users may be confused about `clang modules` and `c++20 modules` and there is already a document about `clang modules`. So I am wondering it might be helpful to explain why we need a new document for `C++20 Modules` instead of `clang modules`.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:64-66
+Things in ``[]`` means optional. The syntax of ``module_name`` and ``partition_name``
+in regex form should be ``[a-zA-Z_][a-zA-Z_0-9.]*``. The dot ``.`` in the name has
+no special meaning.
----------------
h-vetinari wrote:
> > The dot ``.`` in the name has no special meaning.
> 
> Not sure if this is intended to say that the dot in the regex is not needed, or that it has no semantic significance.
> 
> Also, when wanting to match a literal "." in a regex, I'd consider it beneficial for clarity to escape it ("\."), even though it does what's intended in the context of [].
> Not sure if this is intended to say that the dot in the regex is not needed, or that it has no semantic significance.

I mean that it has no semantic significance. For example, the user from python may feel like the dot `.` may be related to `hierarchy`. But it is not the case for C++.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:224-226
+The file name of ``importable module unit`` must end with ``.cppm``
+(or ``.ccm``, ``.cxxm``, etc). The file name of ``module implementation unit``
+should end with ``.cpp`` (or ``.cc``, ``.cxx``, etc).
----------------
h-vetinari wrote:
> As a reader, I have no idea how to parse
> >  must end with ``.cppm`` (or ``.ccm``, ``.cxxm``, etc)
> 
> On the one hand there's a clear restriction ("must"), on the other there's an open-ended list, whose contents are not clear to me.
What I want to say is for `etc` is `usual c++ source file suffix` + `m`. Currently, the list is `.cppm`, `.ccm`, `.cxxm` and `.c++m`. The reason why I put `etc` is that I am afraid of the list may goes longer in the future. But it looks better to state things clear now. I'll follow the suggestion.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:228-230
+The file name of module files should end with ``.pcm``.
+The file name of the module file of a ``primary module interface unit`` should be ``module_name.pcm``.
+The file name of module files of ``module partition unit`` should be ``module_name-partition_name.pcm``.
----------------
h-vetinari wrote:
> Similarly here; can we describe the origin or the restrictions should/must - i.e. do they come from the standard or from the Clang implementation? Is there any enforcement, or do things just error otherwise?
The comes from the clang implementation. If the user don't follow the restrictions, then the clang may fail to build the module. For example, in the "hello world" example, if the name of module file is `M.module` instead of `M.pcm`, the the clang would fail to find the corresponding `M` module.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:238
+
+Another way to specify the dependent module files is to use ``-fmodule-file``.
+
----------------
h-vetinari wrote:
> h-vetinari wrote:
> > Is this equivalent to "-fprebuilt-module-interface". Are there relevant differences, and if so, should the reader of this document know about them?
> Moving this up from further down, but I might be getting things wrong.
Yes, it is true that `-fprebuilt-module-interface` takes a directory and `-fmodule-file` takes a specific file. I'll follow this.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:247
+
+``-fprebuilt-module-interface`` is more convenient and ``-fmodule-file`` is faster since it would save the time for file lookup.
+
----------------
h-vetinari wrote:
>  I didn't understand what would be more convenient at first, but I'm guessing that one takes a directory and the other takes a file? I made a suggestion further up how to reflect this.
>  I'm guessing that one takes a directory and the other takes a file?

Yes.

> I didn't understand what would be more convenient at first

I guess you have understood it. But let me explain it again to avoid any misunderstanding. If there are `n` dependent module files in the same directory, we may need to `n` `-fmodule-file` options, or a `-fprebuilt-module-interface` option. So I said `-fprebuilt-module-interface` is more convenient.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:426-428
+It is not acceptable if we get performance loss after we use modules. The main concern is that when we compile a source file, the compiler need to see the function body
+of imported module units so that the compiler could perform IPO (InterProcedural Optimization, primarily inlining in practice) to optimize functions in current source file
+by the information provided by the imported module units.
----------------
h-vetinari wrote:
> "It is not acceptable" -> "It would be very unfortunate"
> 
> Since it sounds like that's what will happen when switching on optimization?
> Since it sounds like that's what will happen when switching on optimization?

No, it wouldn't happen. I want to say the users of modules won't get worth performance; but they can't get so much speedup either.

What I want to say in the first sentence is that if the compilation process with optimization is the same with the one with debug, the users will get worth performance, which is unacceptable.

I'm not sure if the current wording eliminates the misunderstanding.

================
Comment at: clang/docs/CPlusPlus20Modules.rst:646-647
+
+Another reason is that there are proposals to introduce module mappers to the C++ standard (for example, https://wg21.link/p1184r2).
+Then if we decide to resued Clang's modulemap, we may get in problem once we need to introduce antoher module mapper.
+
----------------
h-vetinari wrote:
> 
> If we decide to Clang's modulemap

If we decide to reuse Clang's modulemap? It looks like it lacks a verb.

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

https://reviews.llvm.org/D131062