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

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

I think we are getting there. I added a few suggestions how to improve the current design.



================
Comment at: clang/include/clang/Interpreter/CodeCompletion.h:15
+#define LLVM_CLANG_INTERPRETER_CODE_COMPLETION_H
+#include "llvm/LineEditor/LineEditor.h"
+
----------------
I still am a bit uncomfortable with relying on a line editor api to address a generic question one could as the interpreter such as "What's visible at that code position"?

Can we not make the overloaded operators outside of the `ReplListCompleter` class. That means moving them in ClangRepl.cpp?


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:42
 class IncrementalParser;
+class CodeCompleteConsumer;
 
----------------
We should put that in its alphabetical order.


================
Comment at: clang/lib/Interpreter/CodeCompletion.cpp:111
+  auto *CConsumer = new ReplCompletionConsumer(Results);
+  auto Interp = Interpreter::createForCodeCompletion(
+      CB, MainInterp.getCompilerInstance(), CConsumer);
----------------
I do not believe we need a spe


================
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) {
----------------
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?


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:304
+llvm::Expected<std::unique_ptr<Interpreter>>
+Interpreter::createForCodeCompletion(
+    IncrementalCompilerBuilder &CB, const CompilerInstance *ParentCI,
----------------
capfredf wrote:
> v.g.vassilev wrote:
> > I still do not entirely understand why we can "just" ask the codecompletion infrastructure for the possible options at the current position. I know that's probably easier set than done but I'd like to entertain that idea for a while..
> I'm a bit confused. Can you expand on the suggestion?
I do not believe we need a special case for creating a CodeCompletionConsumer. I think we could rely on the logic in `CompilerInstance::createCodeCompletionConsumer` which can be triggered in `IncrementalAction::ExecuteAction` just like in `ASTFrontendAction::ExecuteAction`. We will need to copy some of the code over but then you could start the same interpreter infrastructure with slightly different flags.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:6652
+        break;
+      }
+
----------------
Can we test this change separately outside of clang-repl? That would mean creating a tests and run it with `clang -Xclang -fincremental-extensions`


================
Comment at: clang/lib/Parse/Parser.cpp:934
+      PCC = Sema::PCC_Namespace;
+    };
     Actions.CodeCompleteOrdinaryName(
----------------
Likewise, it would be great if we have a test in clang-only mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154382