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