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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked 3 inline comments as done.
iains added a comment.

@ChuanqiXu - I changed the module ownership name to "ModuleDiscardable" - because, while we are permitted to discard these, we might choose not to (to give your better diagnostics) - but we still need to treat them as non-reachable and non-visible.  Please could you examine what is happening with `Modules/explicitly-specialized-template.cpp` where there a multiple error messages for each (intentionally) failing line .. add the test from the std.

@rsmith
 this seems like an awful lot of work that has to be done for every decl we mark used in the module purview - we cannot even short-cut ones that are not in the GMF, since the provisions of [module.global.frag] p3.5 seem to allow this to happen for indirect reaching.  I wonder if I misread the std.
 I am also wondering what is supposed to happen when an interface makes a type reachable, but that type is not visible to the implementation .. it seems to mean that interfaces would have to add using declarations for each such type.



================
Comment at: clang/include/clang/AST/DeclBase.h:240
+    /// GMF is part.
+    ModuleUnreachable
   };
----------------
ChuanqiXu wrote:
> 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.
`ModuleDiscardable` Is better, it says we have permission to discard it (so that it cannot be used or reached) but allows for the case we elect to keep them around for better diagnostics.  You might want to consider renaming the wrapper function similarly?




================
Comment at: clang/include/clang/AST/DeclBase.h:647
+  bool isDiscardedInGlobalModuleFragment() const {
+   return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable;
+  }
----------------
ChuanqiXu wrote:
> 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.
I am very concerned about the amount of work this (GMF decl elision) adds, so would like to minimise this - what I have done is to add an assert at the point we might make a change to the ownership that tests to see the decl is in the fragment we expect.


================
Comment at: clang/lib/Sema/SemaModule.cpp:1009
+      : VD->getASTContext().getUnqualifiedObjCPointerType(VD->getType());
+    T.dump();
+  }
----------------
ChuanqiXu wrote:
> Remove dump
of course, as noted, the patch here is for comment on approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694