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