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

Richard Smith - zygoloid via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 1 18:56:54 PDT 2022

rsmith added inline comments.

Comment at: clang/include/clang/Sema/Sema.h:2275-2276
+  llvm::SmallPtrSet<const Decl *, 32> SeenDecls;
+  llvm::SmallPtrSet<const clang::Type *, 32> SeenTypes;
These names seem too general to live directly in `Sema`.

Comment at: clang/include/clang/Sema/Sema.h:2278
+  /// Examine a type, unwrapping ay decls that might be in the GMF.
+  void findGMFReachableDeclsForType(const Type *T);

Comment at: clang/lib/Sema/SemaModule.cpp:916-920
+            markGMFDeclsReachableFrom(Target, /*MakeVisible*/ true);
+          }
+        } else {
+          markGMFDeclsReachableFrom(Child, /*MakeVisible*/ true);
+        }
Do we need the special `MakeVisible` handling here? I would have thought that it would be sufficient for `Child` itself to be visible. Making the target of a using declaration visible seems like it would do the wrong thing for a case like:

int f();
export module M;
namespace N { export using ::f; }

import M;
int a = f(); // should be an error, ::f should not be visible
int b = N::f(); // should be OK, UsingShadowDecl is visible

I wonder if we need any special handling here at all -- if the GMF decls are referenced by something in an `export` block, then I'd have expected they'll already be marked as used and that marking would have marked them as reachable.

Comment at: clang/lib/Sema/SemaModule.cpp:1024-1036
+  // Only visit each Decl once.
+  if (!SeenDecls.insert(Orig).second)
+    return;
+  // Do not alter the ownership unless it is ModuleDiscardable.
+  if (Orig->isModuleDiscardable()) {
+    assert(Orig->getOwningModule() &&
Instead of storing a `SeenDecls` set and checking it here, is it feasible to check whether we're transitioning this declaration from discardable to retained, and only doing the work below if so?

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list