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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This mostly looks good.

My main concern is adding code to the parser to suppress errors when code completing: this is unlikely to be the only such place. Existing consumers seem happy to ignore errors, if this doesn't work for clang-repl it'd be good to understand why.



================
Comment at: clang/include/clang/Frontend/ASTUnit.h:891
   ///
+  /// \param Act A SyntaxOnlyAction performs actions on the syntax.
+  ///
----------------
This comment doesn't really say anything beyond echoing the name/type, either remove it or add some separate explanation?

e.g. "If Act is specified, it will be used to parse the file. This allows customizing the parse by overriding FrontendAction's lifecycle methods."


================
Comment at: clang/include/clang/Frontend/ASTUnit.h:901
+                    SmallVectorImpl<const llvm::MemoryBuffer *> &OwnedBuffers,
+                    std::function<void(CompilerInstance&)> AfterBeginSourceFile = [](CompilerInstance& CI) -> void {});
 
----------------
capfredf wrote:
> @v.g.vassilev  Not sure if this is the right solution or idiom to extend this method.  
> 
> we can make this high order function more general, like `std::unique_ptr<SyntaxOnlyAction*><()>`
There's no need for this to be a SyntaxOnlyAction, you can take FrontendAction here.

I don't think there's any need to provide a factory, it's safe to assume it will always create one action. (We have FrontendActionFactory in tooling, and it's a pain.)


================
Comment at: clang/include/clang/Interpreter/InterpreterCodeCompletion.h:30
+std::vector<llvm::StringRef>
+ConvertToCodeCompleteStrings(const std::vector<CodeCompletionResult> &Results);
+} // namespace clang
----------------
nit: lowercase initial letter for functions


================
Comment at: clang/include/clang/Interpreter/InterpreterCodeCompletion.h:30
+std::vector<llvm::StringRef>
+ConvertToCodeCompleteStrings(const std::vector<CodeCompletionResult> &Results);
+} // namespace clang
----------------
sammccall wrote:
> nit: lowercase initial letter for functions
you might consider `vector<string>` instead.

this signature doesn't provide any way to return owned names, and so requires that the results are already allocated somewhere as contiguous strings.

This will prevents completing names like `operator int`, as well as some non-names that might prove useful.

This is an interactive operation, copying the strings is unlikely to add significant latency on human scale.


================
Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:342
+    /// Code completion at a top level in a REPL session.
+    CCC_ReplTopLevel,
   };
----------------
v.g.vassilev wrote:
> 
I don't think this name fits with the others, it describes the client rather than the grammatical/semantic context.

I would suggest maybe `CCC_TopLevelOrExpression`?


================
Comment at: clang/lib/Interpreter/InterpreterCodeCompletion.cpp:72
+      break;
+    default:
+      break;
----------------
(up to you, but default between cases seems surprising)


================
Comment at: clang/lib/Parse/ParseDecl.cpp:6648
     if (Tok.is(tok::l_paren)) {
+      if (PP.isIncrementalProcessingEnabled() &&
+          NextToken().is(tok::code_completion)) {
----------------
AIUI, all code completion actions should be ignoring errors, and we should always be happy to cut off parsing and give up as soon as the completion token is reached.

So why do we need special handling for incremental processing here?

(i.e. what behavior do non-repl users see here - if it's correct can clang-repl do the same, if it's wrong why don't we fix both?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154382