[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);
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694