Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal: Make line-index a lib, use nohash_hasher #14733

Merged
merged 40 commits into from
May 6, 2023

Conversation

azdavis
Copy link
Contributor

@azdavis azdavis commented May 4, 2023

These seem like they are not specific to rust-analyzer and could be pulled out to their own libraries. So I did.

azdavis/millet#31

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2023
crates/ide-db/Cargo.toml Outdated Show resolved Hide resolved
lib/non-hash/Cargo.toml Outdated Show resolved Hide resolved
@azdavis azdavis requested a review from Veykril May 4, 2023 06:50
@azdavis azdavis changed the title Add non-hash and line-index libs Make lline-index a lib, use nohash_hasher May 4, 2023
@azdavis azdavis changed the title Make lline-index a lib, use nohash_hasher Make line-index a lib, use nohash_hasher May 4, 2023
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more small thing

crates/stdx/Cargo.toml Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented May 4, 2023

@bors delegate+

@bors
Copy link
Collaborator

bors commented May 4, 2023

✌️ @azdavis can now approve this pull request

lib/line-index/src/lib.rs Outdated Show resolved Hide resolved
lib/line-index/src/lib.rs Outdated Show resolved Hide resolved
lib/line-index/src/lib.rs Outdated Show resolved Hide resolved
lib/line-index/src/lib.rs Show resolved Hide resolved
lib/line-index/src/lib.rs Outdated Show resolved Hide resolved
lib/line-index/src/lib.rs Show resolved Hide resolved
lib/line-index/src/lib.rs Outdated Show resolved Hide resolved
lib/line-index/src/lib.rs Outdated Show resolved Hide resolved
lib/line-index/src/lib.rs Outdated Show resolved Hide resolved
lib/line-index/src/tests.rs Outdated Show resolved Hide resolved
@azdavis azdavis requested a review from matklad May 5, 2023 19:20
lib/line-index/Cargo.toml Outdated Show resolved Hide resolved
@matklad
Copy link
Member

matklad commented May 5, 2023

Sorry, for a somewhat nitpicky review, but I do think it's important that stuff in lib is kept to much higher standard wrt to public APIs than other stuff. As a general rule of thumb, it should never happen that we just move stuff from crates to libs. If we do that, that means one of the following:

  • the crates/ version was needlessly protective about its APIs, we spend too much time on semver for an internal crates
  • the lib/ version has non-robust APIs and will burn through a bunch of major version numbers quickly

@azdavis
Copy link
Contributor Author

azdavis commented May 6, 2023

no problem, I get that the standard for public libs should be higher.

@azdavis azdavis requested a review from matklad May 6, 2023 01:25
@matklad
Copy link
Member

matklad commented May 6, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented May 6, 2023

📌 Commit 60056b8 has been approved by matklad

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 6, 2023

⌛ Testing commit 60056b8 with merge 260e996...

@bors
Copy link
Collaborator

bors commented May 6, 2023

☀️ Test successful - checks-actions
Approved by: matklad
Pushing 260e996 to master...

@bors bors merged commit 260e996 into rust-lang:master May 6, 2023
10 checks passed
@azdavis
Copy link
Contributor Author

azdavis commented May 7, 2023

thanks! could we publish the libs to crates.io? it seems like https://github.com/rust-lang/rust-analyzer/blob/master/.github/workflows/publish-libs.yaml would do it but it runs on pushes to main only (this repo's default branch is not main but master). it also runs on "workflow_dispatch", i'm assuming that's a manual trigger?

bors added a commit that referenced this pull request May 7, 2023
Fix libs publish branch filter

line-index didn't actually get published from #14733, probably because the branch filter was for main but the main branch is called master here. This fixes the workflow file

I also tweaked the libs readme mostly just so the paths filter would pick up the changes.
@lnicola lnicola changed the title Make line-index a lib, use nohash_hasher internal: Make line-index a lib, use nohash_hasher May 7, 2023
@Veykril
Copy link
Member

Veykril commented May 13, 2023

This PR seems to cause panics in r-a now #14792

@azdavis
Copy link
Contributor Author

azdavis commented May 13, 2023

I think the only logic we changed/added are the extra checks to make sure the offset is valid (bounds checks and checking if it’s inside a multi byte character); maybe we could back out those changes until we know more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants