Fix path search #46081
Fix path search #46081
Conversation
GuillaumeGomez
force-pushed
the
fix-path-search
branch
3 times, most recently
from
7d0d016 to
8da48f2
Compare
shepmaster
added
the
S-waiting-on-review
|
This changes the behavior when searching for paths, but it doesn't quite fix the issue? Your PR
Current nightly
Current stable
|
|
Oh damn. Didn't take into account cases where the path doesn't start by the given search... I'll update it. |
GuillaumeGomez
force-pushed
the
fix-path-search
branch
from
8da48f2 to
ad6324f
Compare
|
Ok fixed! The changed line is this one. |
|
After thinking about it once again (and a few tests), I found my solution not good enough so I strongly improved the |
src/librustdoc/html/static/main.js
Outdated
| split.splice(j, 1); | ||
| for (var z = 0; z < split.length; ++z) { | ||
| if (split[z] === "") { | ||
| split.slice(z, 1); |
There was a problem hiding this comment.
Slice returns a new array, so this is a no-op. Did you want to .splice() instead? From the removed code side, it looks like the answer is yes.
More idiomatically, you can also push those that raen't the empty string.
There was a problem hiding this comment.
> x = [1,2,3];
(3) [1, 2, 3]
> x.splice(0, 1)
[1]
> x
(2) [2, 3]
So no, it clearly removes an entry from the array.
There was a problem hiding this comment.
Oh damn, you're absolutely right. Didn't see the missing 'p'!
GuillaumeGomez
force-pushed
the
fix-path-search
branch
from
9ae3a82 to
c00eaa9
Compare
|
Fixed. |
| return MAX_LEV_DISTANCE + 1; | ||
| } | ||
| for (var i = 0; i < path.length; ++i) { | ||
| if (i + startsWith.length > path.length) { |
There was a problem hiding this comment.
You can use the condition used in the if statement as the for loop condition (when inverted), can't you?
There was a problem hiding this comment.
Absolutely. I just have a personal preference to keep loop conditions as simple as possible. If someone else thinks it's a good idea, then I'll just update it.
| } | ||
| lev_total += lev; | ||
| } | ||
| if (aborted === false) { |
There was a problem hiding this comment.
Isn't it more common to check it like this: !aborted
There was a problem hiding this comment.
It's slower. '!' tests (from memory): length !== 0, 0, null, undefined. So clearly, it's way better in js to make the comparison like this.
There was a problem hiding this comment.
Didn't know that. Thanks for explaining it.
There was a problem hiding this comment.
Except you can generally rely on the JIT to figure out that aborted can only contain a boolean.
There was a problem hiding this comment.
I did too much rust, I love being explicit. 😛
|
@bors r+ |
|
📌 Commit c00eaa9 has been approved by |
Fix path search Fixes #46015. r? @QuietMisdreavus
|
☀️ Test successful - status-appveyor, status-travis |
kennytm
added
the
beta-nominated
[beta] Doc search backports This is a backport of #46081, #46175, #46433, and #46672. They all merged cleanly but I haven't tried a build; let's see what Travis says. These PRs fix pretty annoying issues with doc search and so I think it's important they don't slip to stable, but these PRs have *NOT* been `beta-accepted` yet. cc @steveklabnik @GuillaumeGomez can you tag the docs team to talk about beta-acceptance?
|
Looks like we forgot to backport this to 1.23.0 (sorry about that!) so removing beta tags |
alexcrichton
removed
the
beta-nominated
|
Er sorry, looks like this was backported in #46886 |
alexcrichton
added
the
beta-accepted
Fixes #46015.
r? @QuietMisdreavus