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

Mon Jun 13 07:26:55 PDT 2022

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

In D126694#3576853 <https://reviews.llvm.org/D126694#3576853>, @ChuanqiXu wrote:

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

This was not the problem in this particular case - but (for reference) is there an issue for the duplicated diagnostics? - that seems not very user-friendly, especially since C++ emits a lot of long diagnostics already.

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

Yes, fixed with the latest upload; the provisions of [module.global.frag] p3 are quite complex, it's easy to miss one (and now the `explicitly-specialized-template.cpp` tase passes too)

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

That is not the problem I am describing - which more like:

type.h:
typedef int MyInt;

mod-interface.cpp:
module;
#include "type.h"
export module A;

MyInt foo (); // makes MyInt reachable, but not visible.

mod-imp.cpp:
module A;

MyInt foo () { return 42; } // will not compile because the type declaration is not visible.

  I am told that this is the intention .. it just seems a wee bit odd.

================
Comment at: clang/include/clang/AST/DeclBase.h:240
+    /// GMF is part.
+    ModuleUnreachable
   };
----------------
ChuanqiXu wrote:
> 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.
I think we should observe the "as if" rule here, and say that the implementation would behave "as if" the decls were removed (but we choose to leave them, at least for now, because that improves the diagnostic you mentioned before).

The name should be helpful to programmers - if we make it "Discarded" then it will surely be confusing to a programmer to find that the decls are still present - "Discardable" says the right thing (that you should not use them because at some time they could be removed).

================
Comment at: clang/include/clang/AST/DeclBase.h:647
+  bool isDiscardedInGlobalModuleFragment() const {
+   return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable;
+  }
----------------
ChuanqiXu wrote:
> 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.
let's finalise this once we have the rest of the patch in reasonable shape and the approach is agreed to be an acceptable way forward.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694