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