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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Very nice! D60503 <https://reviews.llvm.org/D60503> will conflict, feel free to stall that until this is landed.
On the other hand it will simplify some things, e.g. the prefix is already calculated, and typed scope is available if you want that (no enclosing namespaces though).



================
Comment at: clangd/CodeComplete.cpp:1341
+
+    // Carve out the typed filter from the content so that we don't treat it as
+    // an identifier.
----------------
ioeric wrote:
> sammccall wrote:
> > 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)
> This is following conventions of other sources. Both index and sema provide results for fully-typed names. Considering that users might be already used to this, and completion results tend to give reassurance for correctly typed names, I am inclined to keep the typed filter if it's seen somewhere else in the file.
OK, but we should now be able to just decrement the count for the right entry in the collected identifiers then? rather than carving up the string


================
Comment at: clangd/CodeComplete.cpp:182
 
+// Identifier code completion result.
+struct Identifier {
----------------
This is a bit vague - most results are for identifiers in some sense.
Maybe `RawIdentifier`?


================
Comment at: clangd/CodeComplete.cpp:274
 struct CodeCompletionBuilder {
-  CodeCompletionBuilder(ASTContext &ASTCtx, const CompletionCandidate &C,
+  // ASTCtx can be nullptr if not run with sema.
+  CodeCompletionBuilder(ASTContext *ASTCtx, const CompletionCandidate &C,
----------------
move this comment to the member?
The invariant is important to readers, not just callers of the constructor.


================
Comment at: clangd/CodeComplete.cpp:1177
+
+  int NSema = 0, NIndex = 0, NBoth = 0, NIdent = 0; // Counters for logging.
+  bool Incomplete = false; // Would more be available with a higher limit?
----------------
rename NBoth -> NSemaAndIndex?


================
Comment at: clangd/CodeComplete.cpp:1303
+    // FIXME: initialize ScopeProximity when scopes are added.
+    AllScopes = true; // Identifiers have no scope.
+    auto Style = getFormatStyleForFile(FileName, Content, VFS.get());
----------------
it doesn't make sense to set this here, as long as we're not querying the index.


================
Comment at: clangd/CodeComplete.cpp:1360
         getQueryScopes(Recorder->CCContext, *Recorder->CCSema, Opts);
     if (!QueryScopes.empty())
       ScopeProximity.emplace(QueryScopes);
----------------
add this to the non-sema case too (though the if is always false for now), or add a fixme?
Worried about forgetting this.


================
Comment at: clangd/CodeComplete.cpp:1521
     Relevance.Query = SymbolRelevanceSignals::CodeComplete;
-    Relevance.FileProximityMatch = FileProximity.getPointer();
+    if (FileProximity)
+      Relevance.FileProximityMatch = FileProximity.getPointer();
----------------
why are we not initializing fileproximity in the fallback case?
(To only a single source)


================
Comment at: clangd/CodeComplete.cpp:1565
+        Relevance.Scope = SymbolRelevanceSignals::FileScope;
+        IsIdentifier = true;
+      }
----------------
can we have a SymbolOrigin bit for identifiers, and use that instead?


================
Comment at: clangd/Headers.cpp:180
     return false;
+  if (!HeaderSearchInfo && !InsertedHeader.Verbatim)
+    return false;
----------------
nit: i'd move this above the previous case, it seems both more fundamental and cheaper


================
Comment at: clangd/Headers.cpp:196
+  if (!HeaderSearchInfo)
+    return "\"" + InsertedHeader.File + "\"";
+  std::string Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
----------------
Do we expect this code path to ever be called?
We may want to assert or elog here, depending on how this can be hit.


================
Comment at: clangd/SourceCode.cpp:403
+  llvm::StringMap<unsigned> Identifiers;
+  auto Add = [&Identifiers](llvm::StringRef Identifier) {
+    auto I = Identifiers.try_emplace(Identifier, 1);
----------------
why isn't Add just `++Identifiers[Id]`?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60126