ioeric added a comment.
Herald added a reviewer: jkorous-apple.
Overall looks good! Some ideas about code structure inlined.
================
Comment at: clangd/CodeComplete.cpp:327
+// The CompletionRecorder captures Sema code-complete output, including
context.
+// It filters out ignored results (and therefore does fuzzy-matching of names).
+// It doesn't do scoring or conversion to CompletionItem yet, as we want to
----------------
Does the recorder still do fuzzy-matching?
================
Comment at: clangd/CodeComplete.cpp:339
+ std::vector<CodeCompletionResult> Results;
+ CodeCompletionContext CCContext;
+ Sema *Sema = nullptr; // The Sema that created the results.
----------------
It seems that `CCContext` can change during `ProcessCodeCompleteResults`. It's
unclear what the implication is for `codeCompletionString` at the end.
================
Comment at: clangd/CodeComplete.cpp:434
+ // Returns true if we had more than N candidates, and dropped some.
+ bool more() const { return More; }
----------------
Maybe `dropped()`?
================
Comment at: clangd/CodeComplete.cpp:679
+// - TopN determines the results with the best score.
CompletionList codeComplete(const Context &Ctx, PathRef FileName,
const tooling::CompileCommand &Command,
----------------
The overall behavior looks good. And the comments really help understand the
code! As chatted offline, we might want to break down this function, and a
class that book-keeps all the states might be helpful.
================
Comment at: clangd/CodeComplete.cpp:734
+ // Now we have the needed context to query the index.
+ // FIXME: we shouldn't query the index if the scope is e.g. class members.
+ // FIXME: in addition to querying for extra/overlapping symbols, we should
----------------
I think we already only query namespace scopes now?
================
Comment at: clangd/CodeComplete.cpp:747
+ // FIXME(ioeric): add scopes based on using directives and enclosing ns.
+ if (auto SS = Recorder->CCContext.getCXXScopeSpecifier())
+ Req.Scopes = {getSpecifiedScope(*Recorder->Sema, **SS).forIndex()};
----------------
As discussed offline, we want to defer unqualified completion support (IIRC?)
until we have enough information about visible scopes (i.e. after D42073).
================
Comment at: clangd/CodeComplete.cpp:782
+ // For Sema results, we build the CCS here, where we have the arenas.
+ auto *CCS =
+ Candidate.first.SemaResult
----------------
It would be a bit more natural if the decision of building CCS is hidden in the
candidate. Maybe pass in a callback for creating CCS from a sema result and let
candidate decide whether to create CCS or not? This would also get rid of the
constraint for `build`.
================
Comment at: clangd/CodeComplete.cpp:789
+ auto &R = Output.items.back();
+ R.scoreInfo = Candidate.second;
+ R.sortText = sortText(R.scoreInfo->finalScore, R.label);
----------------
Would it make sense to move the score construction into `build`, by passing in
necessary information in `Candidate.second`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42181
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
