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

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

@chuanqiXu, thanks for reviewing - but it seems I need to find the right direction before dealing with the details.

@rsmith - So here is a revised strategy - it is incomplete, and the code contains debug - so I am posting ****only**** for a review of the direction taken.

I first tried your suggested  `ReachableIfUsed` directly - but that became somewhat tangled because,  as you noted,  `used` is not sufficient - we mark things as used outside of the module purview.

So.

I added `ModuleUnreachable` which is set to be the default module ownership for the GMF.

When we are in module purview and we mark a decl as used or referenced we reset that ownership to visible.
So Sema now has thin wrappers over setIsUsed/markUsed/setReferenced that then handle the case that the decl is marked as `ModuleUnreachable`.

Then comes the **horrible** bit;  AFAICT we then have to walk the decl we just marked to determine if that makes other decls similarly visible - I've made a partial implementation of this (it just covers function decls - which is enough to run the example from the standard).
^^^ this is what I need review of direction-wise .. I cannot at present see a simpler way to do it - or even a reasonable way to cache used-ness dependencies in the GMF.

Any present, I've elected to stream the `ModuleUnreachable` state which is then used to ignore decls in lookup.
It seems harder than I'd expected to 'just not stream` those decls

- although that would be what I'd assume to be the motivation for this facility - reducing the size of PCMs for cases like:

  module;
  #include <something>
  module Foo;
  ....

suggestions most welcome - if the direction is OK, then the `markReachableGMFDecls` function needs to be completed before reviewing the details.



================
Comment at: clang/lib/Sema/Sema.cpp:1154-1155
+
+      for (auto *D : WorkList)
+        DC->removeDecl(D);
+    }
----------------
rsmith wrote:
> rsmith wrote:
> > Please don't remove the declarations from the `DeclContext`; `removeDecl`  is inefficient, not well-tested, and not really compatible with the Clang philosophy of AST fidelity. For example, this will be very problematic for tooling that wants to inspect the translation unit after compilation.
> > 
> > As an alternative that should also fix the issue with checking `isUsed`, how would you feel about this:
> > 
> > * Create a new `ModuleOwnershipKind`, say `ReachableIfUsed`, and set that as the ownership kind for the TU when parsing the global module fragment so it gets inherited into everything we parse in that region. This'll mean that `NextInContextAndBits` needs an extra bit for the ownership field, but I think `Decl` is 8-byte-aligned so that seems fine.
> > * When a declaration is being marked as used, check if its ownership kind is `ReachableIfUsed` and if so change it to `ModulePrivate`.
> > 
> > That should be enough to get the desired semantic effect, and would allow us to get the improved diagnostic experience that @ChuanqiXu asked about. As a potentially separate step,  we can teach the ASTWriter code to (optionally) skip declarations that are `ReachableIfUsed` (and similarly for internal linkage declarations in module interfaces, function bodies for non-inline functions, and anything else we're allowed to omit).
> > When a declaration is being marked as used, check if its ownership kind is ReachableIfUsed and if so change it to ModulePrivate.
> 
> We'd need something a little more subtle, such as checking whether the module ownership kind of the translation unit is no longer `ReachableIfUsed`,  to detect whether we've already left the global module fragment at the point of use. Maybe there's somewhere in `Sema` that marks declarations as used that we can put this logic in instead?
> > When a declaration is being marked as used, check if its ownership kind is ReachableIfUsed and if so change it to ModulePrivate.
> 
> We'd need something a little more subtle, such as checking whether the module ownership kind of the translation unit is no longer `ReachableIfUsed`,  to detect whether we've already left the global module fragment at the point of use. Maybe there's somewhere in `Sema` that marks declarations as used that we can put this logic in instead?

Yes, what I was doing was way too simplistic - please see the top-level comment now on the revised direction.




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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694