Skip to content

internal: Use rustc-hash in more places #16323

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

Closed
wants to merge 1 commit into from

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jan 9, 2024

I think there's still a couple a lot of these left, some in tests and the proc macro server.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2024
@lnicola lnicola marked this pull request as draft January 9, 2024 12:53
@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

Ideally we should add a clippy.toml and the disallowed-types lint with that set up to forbid using plain HashMap, now that lints in Cargo.toml are a thing

@lnicola
Copy link
Member Author

lnicola commented Jan 9, 2024

Good idea, I forgot that was a thing. It feels like the hardest part is bikeshedding them, e.g. what should we use in lsp-server?

@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

Hmm, I guess lsp-server is the exception, it should be fine to use in there?

@lnicola
Copy link
Member Author

lnicola commented Jan 9, 2024

It also doesn't seem to work very well with derive macros. Anyway, I'll give it a try.

@Veykril
Copy link
Member

Veykril commented Jan 10, 2024

what derive macros we use emit hashmap related things? 🤔 or was that more a general note, not specific to r-a's codebase

@lnicola
Copy link
Member Author

lnicola commented Jan 10, 2024

We have some hashmaps in lsp_ext and i think we have to move them into a submodule to allow the lint there, because they derive Serialize.

@Veykril
Copy link
Member

Veykril commented Jan 10, 2024

Might as well just replace those with FxHashMap as well since we have the dependency there.

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2024
@lnicola
Copy link
Member Author

lnicola commented Jan 10, 2024

I don't remember exactly why that didn't work, they might end up in lsp-types or something like that.

@Veykril
Copy link
Member

Veykril commented Jan 10, 2024

I see, well excluding that module should be fine anyways.

@bors
Copy link
Contributor

bors commented Jan 30, 2024

☔ The latest upstream changes (presumably #16394) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril
Copy link
Member

Veykril commented Feb 1, 2024

#16470 supersedes this

@Veykril Veykril closed this Feb 1, 2024
@lnicola lnicola deleted the fxhash branch February 1, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants