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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:1130
     DiagnoseUseOfUnimplementedSelectors();
 
+    // For C++20 modules, we are permitted to elide decls in the Global
----------------
I prefer to wrap this logic to a function to make it easier to read.


================
Comment at: clang/lib/Sema/Sema.cpp:1131-1133
+    // For C++20 modules, we are permitted to elide decls in the Global
+    // Module Fragment of a module interface if they are unused in the body
+    // of the interface.
----------------
I prefer to cite the standard. And the original comment looks not so right (since we don't and couldn't remove a declaration in GMF due to it is not used by the body of the interface)


================
Comment at: clang/lib/Sema/Sema.cpp:1138-1141
+      for (DeclContext::decl_iterator DI = DC->decls_begin(),
+                                      DEnd = DC->decls_end();
+           DI != DEnd; ++DI) {
+        Decl *D = *DI;
----------------
Prefer range based loop.


================
Comment at: clang/lib/Sema/Sema.cpp:1142
+        Decl *D = *DI;
+        Module *M;
+        if (D->isImplicit() || !isa<NamedDecl>(D))
----------------
We could sink the declaration of M to its definition.


================
Comment at: clang/lib/Sema/Sema.cpp:1146
+        M = D->getOwningModule();
+        if (!M || !D->getOwningModule()->isGlobalModule())
+          continue;
----------------
`M == GlobalModuleFragment` says that M is a global module fragment in the current TU explicitly. The original implementation doesn't check if M is in the current TU or not.


================
Comment at: clang/lib/Sema/Sema.cpp:1150
+          continue;
+        if (!D->isUsed(false) && !D->isReferenced())
+          WorkList.push_back(D);
----------------
Should we check for `D->isUsed()` simply? It looks more straight forward to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694