[PATCH] D118893: [C++20][Modules][1/8] Track valid import state.

[Iain Sandoe via Phabricator via cfe-commits]

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