This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix unintentionally loose fuzzy matching, and the tests masking it.
ClosedPublic

Authored by sammccall on Mar 2 2018, 4:48 AM.

Details

Summary

The intent was that [ar] doesn't match "FooBar"; the first character must match
a Head character (hard requirement, not just a low score).
This matches VSCode, and was "tested" but the tests were defective.

The tests expected matches("FooBar") to fail for lack of a match. But instead
it fails because the string should be annotated - matches("FooB[ar]").
This patch makes matches("FooBar") ignore annotations, as was intended.

Fixing the code to reject weak matches for the first char causes problems:

  • [bre] no longer matches "HTMLBRElement". We allow matching against an uppercase char even if we don't think it's head. Only do this if there's at least one lowercase, to avoid triggering on MACROS
  • [print] no longer matches "sprintf". This is hard to fix without false positives (e.g. [int] vs "sprintf"]) This patch leaves this case broken. A future patch will add a dictionary providing custom segmentation to common names from the standard library.

Fixed a couple of index tests that indirectly relied on broken fuzzy matching.
Added const in a couple of missing places for consistency with new code.

Event Timeline

sammccall created this revision.Mar 2 2018, 4:48 AM
sammccall updated this revision to Diff 136719.Mar 2 2018, 4:48 AM

test tweaks

Harbormaster completed remote builds in B15617: Diff 136719.
ilya-biryukov accepted this revision.Mar 5 2018, 1:53 AM

LGTM, but note the unreachable code comment

unittests/clangd/FuzzyMatchTests.cpp
35

Maybe put operator before Word to keep fields grouped together?

37

This code is unreachable. Probably intended to remove return from the previous line?

This revision is now accepted and ready to land.Mar 5 2018, 1:53 AM
This revision was automatically updated to reflect the committed changes.