[PATCH] D126694: [C++20][Modules] Initial implementation of GMF decl elision.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126694#3564629 <https://reviews.llvm.org/D126694#3564629>, @iains wrote:

> the first failure is like this:
>
>   x-h.h:
>   struct A {
>     friend int operator+(const A &lhs, const A &rhs) {
>       return 0;
>     }
>   };
>   
>   X.cpp:
>   module;
>   #include "x-h.h"
>   export module X;
>   
>   export using ::A;
>
> This does not mark *anything* in the GMF (x-h.h) as 'used', and so everything there is unreachable (and hence the fails).
> I.e `export using ::A;` neither marks A as used or referenced.
> I am not sure if we are supposed to special-case this - since `https://eel.is/c++draft/module#global.frag-3.6` explicitly says "In this determination, it is unspecified .. whether `using-declaration, ...  is replaced by the declarations they name prior to this determination,`
> so .. not about how to proceed with this one at present; 
> edit:  but it seems most reasonable to make it behave as if A was itself  exported.

I highly recommend we should mark A here. Maybe we need other interfaces than markDeclUsed and setDeclReferenced. If we don't support this, we couldn't use modules like https://github.com/alibaba/async_simple/blob/CXX20Modules/third_party_module/asio/asio.cppm. This manner is important to use C++20 modules before the build system is ready. Also, I think it is an important tool to implement C++23's std modules. So we should support it.



================
Comment at: clang/include/clang/AST/DeclBase.h:647
+  bool isDiscardedInGlobalModuleFragment() const {
+   return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable;
+  }
----------------
iains wrote:
> ChuanqiXu wrote:
> > Maybe we need to check if the owning module is global module or not.
> The only place that the ownership is `ModuleUnreachable` is in the GMF - so that we do not need to do an additional test.
> 
It is more robust and clear to add the additional check to me. Since the constraint now lives in the comment only. If anyone goes to change it, the codes wouldn't complain.


================
Comment at: clang/include/clang/AST/DeclBase.h:240
+    /// GMF is part.
+    ModuleUnreachable
   };
----------------
Would you like to use the term 'ModuleDiscarded'? My point is that not all unreachable declaration in modules are in GMF. And the term `discard` is used in standard to describe it. So it looks better.


================
Comment at: clang/include/clang/Sema/Sema.h:2273
+  void HandleGMFReachability(Decl *D) {
+    if (D->isModuleUnreachable() && isCurrentModulePurview()) {
+      D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ReachableWhenImported);
----------------
iains wrote:
> ChuanqiXu wrote:
> > I feel better if we would check if D lives in GMF. (We need to insert a check in isDiscardedInGlobalModuleFragment)
> If the consensus is to add an extra test, OK.
> 
> However, as above, specifically to avoid making more and more tests in code that is executed very frequently - as the design currently stands, the only place that  `ModuleUnreachable` is set is in the GMF.
Yeah, my opinion is the same as above. Although it is the design, it is more semantically clear and robust to add a additional check. I am just afraid it would confuse and block other readers or contributors.


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1622
+  if (D->isModuleUnreachable())
+    OS << " ModuleUnreachable";
 }
----------------
iains wrote:
> ChuanqiXu wrote:
> > It may be better to keep the consistent style.
> I don't think that is a matter of style `__module_private__` is a keyword used elsewhere?
> 
> If you look though the file you will see mostly that the printed output does not prepend or append underscores.
> 
> BTW, similar changes are probably needed in other node printers, this was done early to add debug.
Oh, I found `__module_private__ ` is a keyword in clang modules. I didn't recognize it. Even in this case, I still prefer to keep the style consistently. I think users would be more comfortable to read consistent symbols. Also I think it is acceptable to keep `ModuleUnreachable` since it doesn't matter a lot to me.


================
Comment at: clang/lib/Sema/SemaModule.cpp:978
+void Sema::markReachableGMFDecls(Decl *Orig) {
+
+  if (isa<FunctionDecl>(*Orig)) {
----------------
iains wrote:
> ChuanqiXu wrote:
> > It looks better to add assumption as an assertion .
> what is `D` here?
> `markReachableGMFDecls` is only called in the case that `Orig` is **not** discarded (we are marking decls as reachable because they are `used` in the interface..
> 
Oh, `D` should be `Orig`, my bad. Yeah, I found `markReachableGMFDecls ` is only called when `Orig` is not discarded. So I suggest to add the assertion to make it explicit and clear and it could avoid misuse in the future.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:635-636
 
-    if (ModulePrivate) {
-      // Module-private declarations are never visible, so there is no work to
-      // do.
+    if (ModulePrivate ||
+        ModuleOwnership == Decl::ModuleOwnershipKind::ModuleUnreachable) {
+      // Module-private and unreachable declarations are never visible, so
----------------
iains wrote:
> ChuanqiXu wrote:
> > Maybe we could make a new bool variable like `ModulePrivate` to keep the style consistent. So that we could write:
> yes, perhaps we could do that - I am wondering if that code can all be factored into the switch.
It looks possible.


================
Comment at: clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp:32
 void test_early() {
-  in_header = 1; // expected-error {{missing '#include "foo.h"'; 'in_header' must be declared before it is used}}
-  // expected-note@*{{not visible}}
+  in_header = 1; // expected-error {{use of undeclared identifier 'in_header'}}
 
----------------
iains wrote:
> ChuanqiXu wrote:
> > iains wrote:
> > > ChuanqiXu wrote:
> > > > This error message looks worse. I image I could hear users complaining this. (I doesn't say it is your fault since this is specified in standard and the standard cases are harder to understand). I want to know if this is consistent with GCC?
> > > > This error message looks worse. I image I could hear users complaining this. (I doesn't say it is your fault since this is specified in standard and the standard cases are harder to understand). I want to know if this is consistent with GCC?
> > > 
> > > As you say, the standard says "neither reachable nor visible" 
> > > if it's not reachable then. we would not have the information that it was from header foo.h so we cannot form the nicer diagnostic.
> > > 
> > > Perhaps we need to invent "reachable for diagnostics" ... which I'd rather not divert effort to right now.
> > > 
> > Maybe we could make a new diagnostic message.
> I do not think it is a matter of a diagnostic message.
> 
> If we omit the decl from the BMI (which we are permitted to do if it is ModuleUnreachable), then it cannot be found and therefore the information on which header it came from would not be available.
> 
Your word makes sense.


================
Comment at: clang/test/Modules/cxx20-10-4-ex2.cpp:44
+template<typename T> int use_h() {
+  N::X x;		// N::X, N, and ​::​ are decl-reachable from use_­h
+  return h((T(), x));	// N::h is not decl-reachable from use_h, but
----------------
There is invisible symbol.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694