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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

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

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

I've looked into this. The multiple duplicated diagnostic message is a legacy problem. I tried to fix it but it is hard (change it would break many other tests). To filter out the redundant duplicated diagnostic message, you could use '+1' suffix after `expected-error`. For example: https://github.com/llvm/llvm-project/blob/16ca490f450ea3ceaeda92addd8546967af4b2e1/clang/test/Modules/cxx-templates.cpp#L215-L232 BTW, after I filtered out the duplicated diagnostic message, I find the complains the use of `foo<int>` in the module purview, which is not right and should be a bug of this revision.

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

Yeah, and I am not sure what is better idea here. I tried to implement GMF decl elision before. My first idea is similar to your first revision. But I tried to implement indirect discarding that time. So I chose to traverse the decls in GMF until a fixed point when we handle the end of the TU. It looks bad clearly (the time complexity is not satisfying). So I didn't post it up.

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

@rsmith should be the best to answer here. But I am trying to answer it. If  'the implementation' means the implementation unit, I think the type is naturally visible to the implementation unit. We could find the example in: module.interface-example5 <https://eel.is/c++draft/module.interface#example-5> .



================
Comment at: clang/include/clang/AST/DeclBase.h:240
+    /// GMF is part.
+    ModuleUnreachable
   };
----------------
iains wrote:
> 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?
> 
> 
>From my reading, it looks like the std says "we should discard things in following cases... although there are some cases we could make decision by ourself". So it looks like `ModuleDiscarded` is better than `ModuleDiscardable` to me.


================
Comment at: clang/include/clang/AST/DeclBase.h:647
+  bool isDiscardedInGlobalModuleFragment() const {
+   return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable;
+  }
----------------
iains wrote:
> 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.
How about something like:
```
assert (getModuleOwnershipKind() != ModuleOwnershipKind::ModuleDiscardable || getOwningModule()->isGlobalModule);
return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleDiscardable;
```

So that we could get the clear semantics  and the performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694