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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
