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

Reply via email to