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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: clang/lib/Sema/SemaModule.cpp:530-531
 
+  CurContext->addDecl(D);
+  PushDeclContext(S, D);
+
----------------
ChuanqiXu wrote:
> 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.
Ah yes, that makes far more sense. Thanks!


================
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;
----------------
ChuanqiXu wrote:
> 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.
Thanks, I agree that we want to mark the decl invalid rather than leave it seeming valid.


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

https://reviews.llvm.org/D117093