Skip to content

Upstream stdx changes #19512

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

Merged
merged 1 commit into from
Apr 7, 2025
Merged

Conversation

BenjaminBrienen
Copy link
Contributor

I have some changes on my version of stdx that I figured I might as well PR back to r-a.

Let me know if there are things that you want me to undo.
Almost all of the changes come from following clippy lints which are not currently enabled in this workspace.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2025
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.

Why all the random inlines? And most of the must use also seems unnecessary to me (aside from the constructors/builders).

@BenjaminBrienen
Copy link
Contributor Author

I just followed a lint to add the inlines and must_use. must_use is recommended whenever the function doesn't appear to mutate its arguments, so it doesn't make sense to call the function if you don't need the resulting value.

@Veykril
Copy link
Member

Veykril commented Apr 4, 2025

I find putting those on a bunch of functions blindly to be more noise than helpful. We don't run clippy with all pedantic lints precisely because a lot of those lints are subjectively questionable.

As for inline specifically, none of that code is perf critical so we can just rely on the compiler doing the right thing (what it considers right). In fact, a lint telling you about when to use inline makes no sense to me. If the lint knows, the compiler should know either way. So I'd at least like to see the inline attributes removed here. must_use I don't care too much about (as those are reasonable here either way).

@BenjaminBrienen
Copy link
Contributor Author

Let me know if there are any other changes you want me to undo.

@BenjaminBrienen BenjaminBrienen force-pushed the update-stdx branch 3 times, most recently from b44c5e7 to 9967d8b Compare April 4, 2025 16:38
@Veykril
Copy link
Member

Veykril commented Apr 7, 2025

Thanks!

@Veykril Veykril added this pull request to the merge queue Apr 7, 2025
Merged via the queue into rust-lang:master with commit 6832c5a Apr 7, 2025
14 checks passed
@BenjaminBrienen BenjaminBrienen deleted the update-stdx branch April 7, 2025 08:06
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.

3 participants