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

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

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:

```
module;
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694