ChuanqiXu added inline comments.
================ Comment at: clang/lib/Lex/ModuleMap.cpp:910 + return Result; +} + ---------------- The implementation looks similar to `createModuleForInterfaceUnit` really. It looks better to refactor it to `createModuleUnits` (or createModuleForCXX20Modules) which containing an additional argument `Module::ModuleKind`. It would optimize the case for partitions too, which uses `createModuleForInterfaceUnit ` now and it is a little bit odd. ================ Comment at: clang/lib/Sema/SemaModule.cpp:306 + Path[0].second); + // now create the implementation module + Mod = Map.createModuleForImplementationUnit(ModuleLoc, ModuleName, ---------------- I feel like the 3 comments are redundant. ================ Comment at: clang/lib/Sema/SemaModule.cpp:348-352 + // We already potentially made an implicit import (in the case of a module + // implementation unit importing its interface). Make this module visible + // and return the import decl to be added to the current TU. + if (Import) + VisibleModules.setVisible(Import->getImportedModule(), ModuleLoc); ---------------- We could move this logic to the place `Import` is created. ================ Comment at: clang/lib/Sema/SemaModule.cpp:355-357 + // If we made an implicit import of the module interface, then return the + // imported module decl. + return Import ? ConvertDeclToDeclGroup(Import) : nullptr; ---------------- Is it necessary/helpful to return the import declaration? Although there is a FIXME, I think the current method looks a little bit confusing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126959/new/ https://reviews.llvm.org/D126959 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
