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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:1150
+          continue;
+        if (!D->isUsed(false) && !D->isReferenced())
+          WorkList.push_back(D);
----------------
rsmith wrote:
> ChuanqiXu wrote:
> > Should we check for `D->isUsed()` simply? It looks more straight forward to me.
> Does this work properly if the declaration is referenced within the header unit? I think we track whether we've seen any use of the declaration anywhere, not only if we've seen a use in the current translation unit, and we'll need a different mechanism here.
s/header unit/global module fragment/


================
Comment at: clang/lib/Sema/Sema.cpp:1154-1155
+
+      for (auto *D : WorkList)
+        DC->removeDecl(D);
+    }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694