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