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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
