iains marked 5 inline comments as done. iains added inline comments.
================ Comment at: clang/lib/Parse/ParseObjc.cpp:82 if (getLangOpts().Modules || getLangOpts().DebuggerSupport) { - SingleDecl = ParseModuleImport(AtLoc); + Sema::ModuleImportState IS = Sema::ModuleImportState::NotACXX20Module; + SingleDecl = ParseModuleImport(AtLoc, IS); ---------------- ChuanqiXu wrote: > We could remove the IS variable ParseModuleImport takes a reference so that it can update the the ModuleImportState if an invalid state is detected in parsing. So, although the state variable is a dummy here, we cannot remove the IS var. ================ Comment at: clang/lib/Parse/Parser.cpp:913 + } + SingleDecl = ParseModuleImport(SourceLocation(), IS); + } break; ---------------- ChuanqiXu wrote: > I think it is better to: > ``` > SingleDecl = ParseModuleImport(SourceLocation(), > ModuleImportState::NotACXX20Module); > ``` see comment above, ParseModuleImport takes a reference. ================ Comment at: clang/lib/Parse/Parser.cpp:2466 + case Sema::ModuleImportState::NotACXX20Module: + // These cases will be an error when partitions are implemented. + SeenError = false; ---------------- ChuanqiXu wrote: > Generally, we would add a `TODO` or `FIXME` for such comments. > (This is not need to be addressed if the following patch would be landed > quickly) I hope that the main changes will be landed soon, but adding the TODO anyway. ================ Comment at: clang/lib/Sema/SemaModule.cpp:144 GlobalModuleFragment = ModuleScopes.back().Module; // In C++20, the module-declaration must be the first declaration if there ---------------- ChuanqiXu wrote: > I feel better to add an assertion here: > ``` > assert(SeenGMF == GlobalModuleFragment && "msg); > ``` we also have to check that we are in C++20 modules mode, since implicit global module fragments are allowed elsewhere - the test below already guards on C++20 modules. I've made this change, although the assert seems quite heavyweight. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118893/new/ https://reviews.llvm.org/D118893 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits