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

Replace &Option<T> with Option<&T>. #4424

Closed
HaoYang670 opened this issue Nov 29, 2022 · 1 comment · Fixed by #5102 or #5113
Closed

Replace &Option<T> with Option<&T>. #4424

HaoYang670 opened this issue Nov 29, 2022 · 1 comment · Fixed by #5102 or #5113
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@HaoYang670
Copy link
Contributor

HaoYang670 commented Nov 29, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
Actually, I file this issue because I just remember we've ever done the same thing in arrow-rs: apache/arrow-rs#1556
And I find a discussion about the trade-off between &Option<T> and Option<&T>: https://www.reddit.com/r/rust/comments/wqsxk2/is_it_better_to_pass_optiont_or_optiont/

Describe the solution you'd like
A clear and concise description of what you want to happen.
It isn't the goal to replace all the occurrences of &Option<_>, but to do the replacement if we can benefit from it.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
We could not do this

Additional context
Add any other context or screenshots about the feature request here.

@HaoYang670 HaoYang670 added enhancement New feature or request good first issue Good for newcomers labels Nov 29, 2022
@tustvold
Copy link
Contributor

I can't think of a valid reason to ever return &Option<T>, so 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
2 participants