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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:2955-2960
+    FirstDecl,
+    GlobalFragment,
+    ImportAllowed,
+    ImportFinished,
+    PrivateFragment,
+    NotACXX20Module
----------------
I guess it is worth to add comment for these fields. Also I could guess the meaning, I think it is better to comment the semantics accurately.


================
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);
----------------
We could remove the IS variable


================
Comment at: clang/lib/Parse/Parser.cpp:913
+    }
+    SingleDecl = ParseModuleImport(SourceLocation(), IS);
+  } break;
----------------
I think it is better to:
```
SingleDecl = ParseModuleImport(SourceLocation(), ModuleImportState::NotACXX20Module);
```


================
Comment at: clang/lib/Parse/Parser.cpp:2466
+  case Sema::ModuleImportState::NotACXX20Module:
+    // These cases will be an error when partitions are implemented.
+    SeenError = false;
----------------
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)


================
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
----------------
I feel better to add an assertion here:
```
assert(SeenGMF == GlobalModuleFragment && "msg);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118893