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

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

again, thanks for review - but please do not spend any effort on style points yet - the debug and dump stuff is intentionally present this is "for comment on the approach"

i.e. what is important is to establish that this is a reasonable approach.

As of now we have the following fails - which have to be analysed (of course, `markReachableGMFDecls` is expected to be incomplete at this point.

  FAIL: Clang :: CXX/basic/basic.lookup/basic.lookup.argdep/p4-friend-in-reachable-class.cpp (3992 of 15859)
  FAIL: Clang :: Modules/derived_class.cpp (6764 of 15859)
  FAIL: Clang :: Modules/explicitly-specialized-template.cpp (6831 of 15859)
  FAIL: Clang :: Modules/template-function-specialization.cpp (7060 of 15859)
  FAIL: Clang :: Modules/template_default_argument.cpp (7207 of 15859)



================
Comment at: clang/include/clang/AST/DeclBase.h:647
+  bool isDiscardedInGlobalModuleFragment() const {
+   return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable;
+  }
----------------
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.



================
Comment at: clang/include/clang/Sema/Sema.h:2273
+  void HandleGMFReachability(Decl *D) {
+    if (D->isModuleUnreachable() && isCurrentModulePurview()) {
+      D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ReachableWhenImported);
----------------
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.


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1622
+  if (D->isModuleUnreachable())
+    OS << " ModuleUnreachable";
 }
----------------
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.


================
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) {
----------------
ChuanqiXu wrote:
> 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.
this code has been removed (it belongs in the D126959 patch).

(not relevant to the patch, but for the record), for the functionality to be correct - we must ensure that the interface module is registered first - so that the module names table contains the pointer to that


================
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
----------------
ChuanqiXu wrote:
> So that we could hoist the codes.
this has been reorganised


================
Comment at: clang/lib/Sema/SemaModule.cpp:978
+void Sema::markReachableGMFDecls(Decl *Orig) {
+
+  if (isa<FunctionDecl>(*Orig)) {
----------------
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..



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


================
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'}}
 
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694