[PATCH] D154382: [ClangRepl] support code completion at a REPL

[Fred Fu via Phabricator via cfe-commits]

capfredf marked 5 inline comments as done.
capfredf added inline comments.


================
Comment at: clang/lib/Interpreter/CodeCompletion.cpp:111
+  auto *CConsumer = new ReplCompletionConsumer(Results);
+  auto Interp = Interpreter::createForCodeCompletion(
+      CB, MainInterp.getCompilerInstance(), CConsumer);
----------------
v.g.vassilev wrote:
> I do not believe we need a spe
I am not sure I am following.
For a regular interpreter, create Interpreter -> create Incremental Parser -> IncrmentalASTAction triggered -> set a default code completion consumer(PrintCodeCompletionConsumer) to CI if no code consumer has been set -> Sema initialized.
Based on this logic, we need to provide our CodeConsumer so that IncrementalASTAction does not create the default, plus we need to a parent CI to a new code completion interpreter. I don't see how we can avoid a special code path for creation


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:324
 
-llvm::Expected<PartialTranslationUnit &>
-IncrementalParser::Parse(llvm::StringRef input) {
+llvm::Error IncrementalParser::ParseForCodeCompletion(llvm::StringRef input,
+                                               size_t Col, size_t Line) {
----------------
v.g.vassilev wrote:
> Is there any other particular reason for this routine to be different rather than `PP.SetCodeCompletionPoint(*FE, Line, Col)`? That is, can we merge back the old code and only set the point of completion externally?
The regular `Parse` does some lexing after parsing, which I don't think is necessary for code completion. 

Also, this is more or less because, as a functional programmer, I prefer building things using small functions with explicit arguments.  

I'll try if I can use the old parse with the completion point set and tested. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154382