bruno accepted this revision.
bruno added a comment.
This revision is now accepted and ready to land.
LGTM with some minor comment below!
================
Comment at: clang/include/clang/Basic/DiagnosticSerializationKinds.td:78
+def err_module_file_missing_definition : Error<
+ "module file '%0' is missing the main module's definition">, DefaultFatal;
----------------
dexonsmith wrote:
> bruno wrote:
> > aprantl wrote:
> > > Should this be `AST file` like in the above error message?
> > +1
> I think this should be `module file`, because it only applies to modules.
> Notice that err_module_file_not_found and err_module_file_out_of_date use
> `%select{PCH|module|AST} file`; this is just a constant-folding of that.
Makes sense!
================
Comment at: clang/lib/Serialization/ASTReader.cpp:5503
+ F.DidReadMainModule = true;
CurrentModule->setASTFile(F.File);
----------------
dexonsmith wrote:
> bruno wrote:
> > Is this enough? Because this is done at the end of `SUBMODULE_DEFINITION`,
> > could it be that only some of the submodules were read (top level or not)
> > and this is still going to be true? I wonder if marking this `true` at the
> > end of successful `SUBMODULE_BLOCK` instead wouldn't be a better option.
> I see three cases to distinguish between:
>
> 1. An error is encountered somewhere in the submodule block.
> 2. The submodule block is read without error, but the main module is not
> seen/added.
> 3. The submodule block is read without error, and the main module is
> seen/added.
>
> Setting a flag to `true` at the end of a successful `SUBMODULE_BLOCK` would
> help to distinguish between (1) and (2 or 3), but the return status already
> tells us that, and `ReadAST` already deletes all the just-loaded modules in
> that case.
>
> This patch as-is helps to distinguish between (2) and (3) once the early
> return for (1) has been avoided.
>
> Does that make sense? Or maybe I didn't follow your suggestion.
Oh, I see now! Thanks for the explanation, perhaps add this to the commit
message?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70063/new/
https://reviews.llvm.org/D70063
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
