[PATCH] D70063: clang/Modules: Error if ReadASTBlock does not find the main module

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith marked 2 inline comments as done.
dexonsmith added a comment.

Committed 83dcb34b6bf4c175040b18d3e8c3c468418009fc <https://reviews.llvm.org/rG83dcb34b6bf4c175040b18d3e8c3c468418009fc>.  Thanks for the reviews!



================
Comment at: clang/lib/Serialization/ASTReader.cpp:5503
 
+        F.DidReadMainModule = true;
         CurrentModule->setASTFile(F.File);
----------------
bruno wrote:
> 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? 
Added a comment there, and also reworded variables / diagnostics from talking about "main module" to talking about "top-level submodule", since I think that might be more consistent (and maybe more clear).


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

https://reviews.llvm.org/D70063