[PATCH] D104918: [clang-repl] Implement partial translation units and error recovery.

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.h:690-691
+
+  /// The translation unit is a partial translation unit, growing incrementally.
+  TU_Partial
 };
----------------
rsmith wrote:
> I don't think this is clear enough about the difference between `TU_Prefix` and `TU_Partial`. I think the difference is:
> 
> * `TU_Prefix` is an incomplete prefix of a translation unit. Because it's not complete, we don't perform (most) finalization steps (eg, template instantiation) at the end.
> * `TU_Partial` is a complete translation unit that we might nonetheless incrementally extend later. Because it is complete (and we might want to generate code for it), we do perform template instantiation, but because it might be extended later, we don't warn if it declares or uses undefined internal-linkage symbols.
> 
> I wonder if `TU_Incremental` would be a better name than `TU_Partial`.
Good point. Fixed.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:63
+    if (auto Err = ErrOrPTU.takeError())
       return Err;
+    if (ErrOrPTU->TheModule)
----------------
sgraenitz wrote:
> `ErrorOr<T>` has different semantics and is still in use, so the name could be confusing. Idiomatic usage for `Expected<T>` would rather be like:
> ```
> auto PTU = Parse(Code);
> if (!PTU)
>   return PTU.takeError();
> ```
> 
> The patch didn't introduce it, but it might be a good chance for improvement.
Ha, that made the code much nicer looking! Thanks!!


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:178
+    TranslationUnitDecl *PreviousTU = MostRecentTU->getPreviousDecl();
+    assert(PreviousTU);
+    TranslationUnitDecl *FirstTU = MostRecentTU->getFirstDecl();
----------------
sgraenitz wrote:
> Where does it point, if the very first TU fails? Maybe worth noting in the assert and/or adding a test case?
The very first TU contains the compiler builtins and it is created when the ASTContext is created. Not having a PreviousTU would mean that we did not initialize the CompilerInstance properly. 

Added some description in the assert.


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

https://reviews.llvm.org/D104918