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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
