nridge marked 6 inline comments as done.
nridge added a comment.
I'm posting some partial responses because I have some questions (really just
one, about fuzzy matching).
In general the comments seem reasonable and I plan to address all of them.
(I've marked some comments as done because I've addressed them locally. I'm not
uploading a revised patch yet because it wouldn't be very interesting.)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:226
+
+std::vector<LocatedSymbol> navigationFallback(ParsedAST &AST,
+ const SymbolIndex *Index,
----------------
sammccall wrote:
> this function should have a high-level comment describing the strategy and
> the limitations (e.g. idea of extending it to resolve nearby matching tokens).
>
> A name like `locateSymbolNamedTextuallyAt` would better describe what this
> does, rather than what its caller does.
>
> I would strongly consider exposing this function publicly for the detailed
> tests, and only smoke-testing it through `locateSymbolAt`. Having to break
> the AST in tests or otherwise rely on the "primary" logic not working is
> brittle and hard to verify.
> I would strongly consider exposing this function publicly for the detailed
> tests, and only smoke-testing it through locateSymbolAt. Having to break the
> AST in tests or otherwise rely on the "primary" logic not working is brittle
> and hard to verify.
I was going to push back against this, but I ended up convincing myself that
your suggestion is better :)
For the record, the consideration that convinced me was:
Suppose in the future we add fancier AST-based logic that handles a case like
`T().foo()` (for example, by surveying types for which `T` is actually
substituted, and offering `foo()` inside those types). If all we're testing is
"navigation works for this case" rather than "navigation works for this case
//via the AST-based mechanism//", we could regress the AST logic but have our
test still pass because the testcase is simple enough that the text-based
navigation fallback (that we're adding here) works as well.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:252
+ Index->fuzzyFind(Req, [&](const Symbol &Sym) {
+ auto Loc = symbolToLocation(Sym, MainFilePath);
+ if (!Loc) {
----------------
sammccall wrote:
> I'm not sure why we're using SymbolToLocation here:
> - Main file URI check: the `Symbol` has URIs. They need to be canonicalized
> to file URIs before comparison. This allows checking both decl and def
> location.
> - PreferredDeclaration and Definition can be more easily set directly from
> the `Symbol`
Well the `Symbol` has `SymbolLocation`s and we need protocol `Location`s, so we
have to use something to convert them. Other places that perform such
conversion use `symbolToLocation()`, so I reused it.
But you're right that `symbolToLocation()` also has some "pick the definition
or the declaration" logic which is less appropriate here. I can factor out the
`SymbolLocation` --> `Location` conversion logic from `symbolToLocation()`, and
just use that here.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:258
+
+ // For now, only consider exact name matches, including case.
+ // This is to avoid too many false positives.
----------------
sammccall wrote:
> I wouldn't bother qualifying this as "for now". Any code is subject to change
> in the future, but requiring an exact name match for index-based results
> seems more like a design decision than a fixme.
Do we want to rule out the possibility of handling typos in an identifier name
in a comment (in cases where we have high confidence in the match, e.g. a long
/ unique name, small edit distance, only one potential match) in the future?
This is also relevant to whether we want to keep the `FuzzyMatcher` or not.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72874/new/
https://reviews.llvm.org/D72874
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
