[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Sam McCall via Phabricator via cfe-commits]

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