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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

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

> so standard 10.4 ex 2 gives the expected output with this patch stack

Cool, and I think we should add that one as a test if it works. Tests are always good.

---

BTW, I guess we could remove dependencies with D126959 <https://reviews.llvm.org/D126959> , D126189 <https://reviews.llvm.org/D126189>, D126691 <https://reviews.llvm.org/D126691>. If I understand things correctly, there shouldn't be dependencies between them.



================
Comment at: clang/include/clang/AST/DeclBase.h:647
+  bool isDiscardedInGlobalModuleFragment() const {
+   return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable;
+  }
----------------
Maybe we need to check if the owning module is global module or not.


================
Comment at: clang/include/clang/Sema/Sema.h:2273
+  void HandleGMFReachability(Decl *D) {
+    if (D->isModuleUnreachable() && isCurrentModulePurview()) {
+      D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ReachableWhenImported);
----------------
I feel better if we would check if D lives in GMF. (We need to insert a check in isDiscardedInGlobalModuleFragment)


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1622
+  if (D->isModuleUnreachable())
+    OS << " ModuleUnreachable";
 }
----------------
It may be better to keep the consistent style.


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1636
+  if (D->isModuleUnreachable())
+    OS << " ModuleUnreachable";
   if (D->isFixed())
----------------
ditto


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1647
+  if (D->isModuleUnreachable())
+    OS << " ModuleUnreachable";
   if (D->isCompleteDefinition())
----------------
ditto


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1679
+  if (D->isModuleUnreachable())
+    OS << " ModuleUnreachable";
 
----------------
ditto


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1756
+  if (D->isModuleUnreachable())
+    OS << " ModuleUnreachable";
 }
----------------
ditto


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1778
+  if (D->isModuleUnreachable())
+    OS << " ModuleUnreachable";
   if (D->isNRVOVariable())
----------------
ditto


================
Comment at: clang/lib/Sema/SemaModule.cpp:265
+  Module *Mod; // The module we are creating.
+  Module *Interface = nullptr; // The interface fir an implementation.
   switch (MDK) {
----------------
Given `Interface` is only assigned in the case of `ModuleDeclKind::Implementation`, it looks possible to sink the declaration to the place it get assigned. In this way, we could avoid the confusion here.


================
Comment at: clang/lib/Sema/SemaModule.cpp:355-369
+  if (Interface) {
+    // Make the import decl for the interface.
+    ImportDecl *Import = ImportDecl::Create(Context, CurContext, ModuleLoc,
+                                            Interface, Path[0].second);
+    VisibleModules.setVisible(Interface, ModuleLoc);
+    if (auto *InterfaceGMF = Interface->getGlobalModuleFragment())
+      // The fact that the GMF is a seaprate sub-module is an implementation
----------------
So that we could hoist the codes.


================
Comment at: clang/lib/Sema/SemaModule.cpp:366
-  // imported module decl.
-  return Import ? ConvertDeclToDeclGroup(Import) : nullptr;
 }
----------------
This comes from https://reviews.llvm.org/D126959. I feel like odd to return the import declaration that time. I feel like it is better to return the module declaration itself instead of the imported one. Let's return nullptr now and fix it in the future.


================
Comment at: clang/lib/Sema/SemaModule.cpp:964-965
+  if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(S)) {
+    auto *D = DR->getFoundDecl();
+//    llvm::dbgs() << "DR:"; D->dump();
+    if (D->isModuleUnreachable()) {
----------------



================
Comment at: clang/lib/Sema/SemaModule.cpp:978
+void Sema::markReachableGMFDecls(Decl *Orig) {
+
+  if (isa<FunctionDecl>(*Orig)) {
----------------
It looks better to add assumption as an assertion .


================
Comment at: clang/lib/Sema/SemaModule.cpp:979-980
+
+  if (isa<FunctionDecl>(*Orig)) {
+    auto *FD = cast<FunctionDecl>(Orig);
+    for (auto *P : FD->parameters()) {
----------------



================
Comment at: clang/lib/Sema/SemaModule.cpp:996-999
+//      if (Changed) {
+//       llvm::dbgs() << "P:"; P->dump();
+//      }
+    }
----------------



================
Comment at: clang/lib/Sema/SemaModule.cpp:1003-1004
+      FindDeclRefExprs(S);
+    }
+  } else if (isa<VarDecl>(*Orig)) {
+    auto *VD = cast<VarDecl>(Orig);
----------------
clang prefer shorter indentation.


================
Comment at: clang/lib/Sema/SemaModule.cpp:1009
+      : VD->getASTContext().getUnqualifiedObjCPointerType(VD->getType());
+    T.dump();
+  }
----------------
Remove dump


================
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
----------------
Maybe we could make a new bool variable like `ModulePrivate` to keep the style consistent. So that we could write:


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:649-652
+  } else if (ModuleOwnership == Decl::ModuleOwnershipKind::ModuleUnreachable) {
+    D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModuleUnreachable);
   } else if (ModulePrivate) {
     D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
----------------



================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694