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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked 8 inline comments as done.
ChuanqiXu added a comment.

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

> Thanks! Repeating a point that might have been overlooked from D131062 <https://reviews.llvm.org/D131062> (this time not as comments in the diff to avoid the "pollution" that caused the move to this PR):
>
>> If you do open a new revision, please also consider breaking the lines at a length that phabricator doesn't overflow (seems to be 122 characters), and change all occurrences of "codes" to "code".

Yeah, I think I've addressed it.

In D131388#3706080 <https://reviews.llvm.org/D131388#3706080>, @iains wrote:

> general comment.
>
> Do we encourage contractions (don't, can't) etc. in documentation?
> I would suggest that to assist in any translation process it is better to write "do not" or "can not" instead (but that's just an opinion, not a matter of correctness).

Yes. I've used many `must`/`should` in the document. Maybe there is someplace missing.



================
Comment at: clang/docs/CPlusPlus20Modules.rst:25-26
+Although the term ``modules`` has a unique meaning in C++20 Language Specification,
+when people talk about C++20 modules, they may refer to another C++20 feature:
+header units. Therefore, this document will try to cover header units as well.
+
----------------
iains wrote:
> this also includes `C++20 header units`, which are also covered in this document.
> 
I took half of the suggestion. Since I still want to clarify that the two terms  `modules` and `header units` are different.


================
Comment at: clang/docs/CPlusPlus20Modules.rst:219
+Currently, C++20 Modules are enabled automatically if the language standard is ``-std=c++20`` or newer.
+The ``-fmodules-ts`` option is deprecated and is planned to be removed.
+
----------------
iains wrote:
> procedural point: do we ever actually remove an existing option - or does it just become a "NOP"?
According to the history, we did  actually remove some option: https://github.com/llvm/llvm-project/commit/29f852a1516bcd3928dad74835965f238de34409


================
Comment at: clang/docs/CPlusPlus20Modules.rst:673-674
+
+Another reason is that there are proposals to introduce module mappers to the C++ standard (for example, https://wg21.link/p1184r2).
+If we decide to reuse Clang's modulemap, we may get in trouble once we need to introduce another module mapper.
+
----------------
iains wrote:
> as an aside : 
> 
> there is an open question in the implementation of p1184r2 as to whether one form of input that the module mapper could consume would be module map files (but, obviously, producing C++20 compliant output).
> 
> I wonder if this section is adding information that is useful to the user ? 
> (perhaps it is more documentation of implementation decisions?)
> 
> As noted before `semantics of clang module headers != semantics of C++20 header units` seems a sufficient reason for keeping them separate?
> 
> I am not sure what could change in the future to alter this, since existing code will have the current semantics, even //if// some change is later made to the semantics of clang header modules ?
> 
> I wonder if this section is adding information that is useful to the user ? 
(perhaps it is more documentation of implementation decisions?)

I think this section is helpful to eliminate the confusion of the users. Since the users may be confused (or even complain) why don't we reuse the existing tools.

> As noted before semantics of clang module headers != semantics of C++20 header units seems a sufficient reason for keeping them separate?
>
> I am not sure what could change in the future to alter this, since existing code will have the current semantics, even if some change is later made to the semantics of clang header modules ?

To be honest, I think the differences mentioned above could be handled by compiler internally. (maybe a lot of `if (isHeaderUnit)....`). Or even the folks of clang modules could get involved in and say "OK, we could accept the cost." But it is a strong argument that it is possible to introduce **new** module mappers.  So the current decision is much more comprehensible to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131388