[PATCH] D117093: [C++20] [Modules] Exit early if export decl is invalid

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/Sema/SemaModule.cpp:530-531
 
+  CurContext->addDecl(D);
+  PushDeclContext(S, D);
+
----------------
aaron.ballman wrote:
> Am I understanding properly that this moved up here so that the for loop on line 553 can traverse the new context?
> 
> If so, can it be moved down to immediately before the for loop?
The intention to move these up is to make sure D could be put in the context even error detected. It wouldn't affect the traverse on line 556 since an ExportDecl wouldn't be a NamespaceDecl.


================
Comment at: clang/lib/Sema/SemaModule.cpp:539-550
+    return D;
   } else if (!ModuleScopes.back().ModuleInterface) {
     Diag(ExportLoc, diag::err_export_not_in_module_interface) << 1;
     Diag(ModuleScopes.back().BeginLoc,
          diag::note_not_module_interface_add_export)
         << FixItHint::CreateInsertion(ModuleScopes.back().BeginLoc, "export ");
+    return D;
----------------
aaron.ballman wrote:
> It seems a bit suspicious to me that we call `D->getInvalidDecl()` below within the for loop, but all the other places we leave it as a valid declaration despite it causing error diagnostics.
Yeah, it is intentional to call `D->setInvalidDecl()` this loop. It would suppress more diagnostic messages. But I feel it is good to call `D->setInvalidDecl()` in other places.


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

https://reviews.llvm.org/D117093