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

simd: reduce pub functions visibility #182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Sep 2, 2024

This tweaks all SIMD `pub` functions, moving them to `pub(crate)`
instead.
@AaronO
Copy link
Contributor

AaronO commented Sep 2, 2024

What's the broader rationale ?

Wouldn't this "violate" redundant_pub_crate ?

@seanmonstar
Copy link
Owner

FWIW, I am not convinced by the description at the clippy lint at all.

Why is this bad?

Writing pub(crate) is misleading when it’s redundant due to the parent module’s visibility.

There's nothing misleading about it. It doesn't cause any harm.

@AaronO
Copy link
Contributor

AaronO commented Sep 2, 2024

There's nothing misleading about it. It doesn't cause any harm.

You could say it doesn't "harm", but in this instance it doesn't solve or address anything either ...
(I'd consider it if it served some clear ulterior motive, but at face value seems like a pointless change)

I'd argue it's somewhat a responsibility leak... Do we care about inner interface visibility much or predominantly the one exposed by lib.rs ? mod simd; already covers this, it's redundant IMO.

@lucab
Copy link
Contributor Author

lucab commented Sep 2, 2024

Apologies, I should've added some breadcrumbs upfront; I've update the PR description with some refs now.

I think this rustc builtin lint description describes it better than what I could do with my own words. Unfortunately it is not enforced by default due to historical reasons (pub(_) did not exist until 1.18).
I personally did this locally to lower the cognitive load, not having to check for possible API breakages in the middle of other refactors.

I can't speak for the clippy lint, however I did notice that it is in nursery (it has been demoted there 4 years ago) and with a bunch of open issues and people voicing for its removal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants