sammccall added a comment.
I'd like to sync up briefly on https://github.com/clangd/clangd/issues/241 so
we know where we want to end up.
I think this is in good shape and certainly doesn't need a bigger scope, just
want to be able to reason about how things will fit together.
================
Comment at: clang-tools-extra/clangd/SourceCode.h:93
+/// the entire comment or string token.
+SourceRange getWordAtPosition(const Position &Pos, const SourceManager &SM,
+ const LangOptions &LangOpts);
----------------
nridge wrote:
> sammccall wrote:
> > sammccall wrote:
> > > consider moving the isLikelyToBeIdentifier check inside. The current API
> > > is pretty general and it's not clear yet what (else) it's good for so
> > > it's nice to direct towards intended usage.
> > >
> > > Also doing the identifier check inside this function is more convenient
> > > when it relies on markers outside the identifier range (like doxygen `\p`
> > > or backtick-quoted identifiers)
> > >
> > > That said, you may still want to return the range when it's not a likely
> > > identifier, with a signature like `StringRef getWordAtPosition(bool
> > > *LikelyIdentifier = nullptr)`. I'm thinking of the future case where the
> > > caller wants to find a nearby matching token and resolve it - resolving
> > > belongs in the caller so there's not much point having this function
> > > duplicate the check.
> > This doesn't use the SourceManager-structure of the file, so the natural
> > signature would be `StringRef getWordAtPosition(StringRef Code, unsigned
> > Offset)`.
> >
> > (what are the practical cases where langopts is relevant?)
> Now that I'm using `wordTouching()` from D75479, I think this comment no
> longer applies?
I think the reasons still apply - D75479 doesn't need to check likelihood (it
considers actual use as identifier evidence enough) so I didn't include it
there, but we should eventually merge these more thoroughly I think. No need to
do that until we actually want to implement different heuristics though.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:240
+ Req.ProximityPaths = {MainFilePath};
+ // FIXME: Set Req.Scopes to the lexically enclosing scopes.
+ // For extra strictness, consider AnyScope=false.
----------------
nridge wrote:
> sammccall wrote:
> > FWIW the API for this is visibleNamespaces() from SourceCode.cpp.
> > (No enclosing classes, but I suspect we can live without them once we have
> > a nearby-tokens solution too)
> Thanks, that's convenient!
>
> Out of curiosity, though: is the reason to prefer this lexer-based approach
> over hit-testing the query location against `NamespaceDecl`s in the AST,
> mainly for performance?
Well, it was written for fallback code completion when we have no AST at all :-)
Gathering from the AST should be better, though it's not quite as simple as
hit-testing (you also have to find `using namespace`). But this exists today,
which is a feature!
================
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.
----------------
nridge wrote:
> 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.
No idea whether typo-correction is a good idea in principle - tradeoff between
current false negatives and false positives+compute.
However neither FuzzyMatcher nor the existing index implementations support/can
easily support real typo correction, and it seems implausible to me we'd add it
for this feature.
Compare to e.g:
- allowing case-insensitive match in some cases: `fooBar` vs `FooBar` is a
plausible "typo". This is easy to implement.
- correct the typo where we see the fixed version used as an identifier in
this file (and not the original). Excludes some cases, but drives
false-positives way down, and easy to implement.
I don't think we need to rule things out, but I'm uncertain enough about the
approach to think that putting comments, fuzzymatcher etc here speculatively
isn't worth it.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:489
+ Index->fuzzyFind(Req, [&](const Symbol &Sym) {
+ auto MaybeDeclLoc =
+ symbolLocationToLocation(Sym.CanonicalDeclaration, MainFilePath);
----------------
nridge wrote:
> Sorry this location-setting code is so messy. All my attempts to make it more
> concise have been thwarted by `llvm::Expected`'s very restrictive API.
Ugh, don't get me started on Error/Expected :-(
I'd love to get rid of it somehow, but it seems like we'd inevitably just end
up with the new thing + Error/Expected + error_code/ErrorOr + return-a-bool,
and I'm not sure it'd be better.
(If you have more energy than me, I'd enthusiastically +1 an llvm-dev proposal
to drop the clever checks from llvm::Error, and I know some others who would...)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72874/new/
https://reviews.llvm.org/D72874
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
