sammccall added inline comments.
================
Comment at: clangd/CodeComplete.cpp:1136
+std::unique_ptr<SymbolIndex>
+indexIdentifiers(llvm::StringRef FileName, llvm::StringRef Content,
+ const format::FormatStyle &Style) {
----------------
as discussed offline, I love the lexer approach but returning it as an index
seems like the wrong level of abstraction.
These are just strings, and if we want to tweak e.g. the scoring or merging of
these we'll be in a better position to do so if we don't pretend they have
Symbol semantics (their own signals, ids, etc).
So I'd suggest this fn should return something like `StringSet<>` or
`StringMap<unsigned>` for counts.
Can we move it to `SourceCode.h` too?
================
Comment at: clangd/CodeComplete.cpp:1136
+std::unique_ptr<SymbolIndex>
+indexIdentifiers(llvm::StringRef FileName, llvm::StringRef Content,
+ const format::FormatStyle &Style) {
----------------
sammccall wrote:
> as discussed offline, I love the lexer approach but returning it as an index
> seems like the wrong level of abstraction.
> These are just strings, and if we want to tweak e.g. the scoring or merging
> of these we'll be in a better position to do so if we don't pretend they have
> Symbol semantics (their own signals, ids, etc).
> So I'd suggest this fn should return something like `StringSet<>` or
> `StringMap<unsigned>` for counts.
>
> Can we move it to `SourceCode.h` too?
I don't think we need FileName as a parameter - it shouldn't affect the return
value, so we can just use a dummy value.
================
Comment at: clangd/CodeComplete.cpp:1209
+
+ // The following fields are initialized once Sema runs or run without
compile.
+ CodeCompletionContext::Kind CCContextKind = CodeCompletionContext::CCC_Other;
----------------
I'm not sure these changes (mostly to comment formatting) improve clarity.
I think it would be enough to add a comment inside the top of runWithoutCompile
saying `// Fill in fields normally set by runWithSema()` or so
================
Comment at: clangd/CodeComplete.cpp:1212
+ llvm::Optional<FuzzyMatcher> Filter;
+ Range TextEditRange;
+ std::vector<std::string> QueryScopes;
----------------
ReplacedRange?
================
Comment at: clangd/CodeComplete.cpp:1318
+ CodeCompleteResult
+ runWithoutCompile(llvm::StringRef Content, Position Pos,
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) && {
----------------
runWithoutSema maybe? - compile is a fine name but this file calls this concept
sema for whatever reason
================
Comment at: clangd/CodeComplete.cpp:1320
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) && {
+ auto CompletionFilter = speculateCompletionFilter(Content, Pos);
+ if (!CompletionFilter) {
----------------
hmm, this function should likely also move to SourceCode, use Offset instead of
Position, and move to SourceCode.h. But probably a different patch.
================
Comment at: clangd/CodeComplete.cpp:1341
+
+ // Carve out the typed filter from the content so that we don't treat it as
+ // an identifier.
----------------
you could just erase the typed filter from the suggestion list.
(It may be a valid word spelled elsewhere, but there's no point suggesting it)
================
Comment at: clangd/CodeComplete.cpp:1357
+ auto Results = queryIndex(*Idx);
+ return toCodeCompleteResult(mergeResults(/*SemaResults=*/{}, Results));
+ }
----------------
I suspect you want to pass the filtered strings through to mergeResults and
have it/CompletionCandidate do the scoring.
================
Comment at: clangd/CodeComplete.h:241
+CodeCompleteResult
+codeCompleteNoCompile(PathRef FileName, llvm::StringRef Content, Position Pos,
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
----------------
It seems a little odd to have a different entrypoint here.
Could we simply indicate this with Preamble=nullptr?
It seems it might even make sense to use the same speculative fuzzyfind?
So I think PCHContainerOperations would be the only odd one out. I'm going to
send a patch to remove that anyway :-)
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60126/new/
https://reviews.llvm.org/D60126
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
