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

Must use #44

Closed
wants to merge 3 commits into from
Closed

Must use #44

wants to merge 3 commits into from

Conversation

Hoverbear
Copy link
Collaborator

Fixes #40 .

I'd love to hear more opinions on this! cc @djc @Boscop

Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear self-assigned this Nov 16, 2019
@djc
Copy link
Contributor

djc commented Nov 16, 2019

I'm... unconvinced. As far as I know must_use is mostly used for things like Result or Future where not using the value with a large probability constitutes a logical error, and so it is not usually applied to all the getters. Here, instead (as far as I can tell from a quick review), it results merely in an unused value -- so not sure this adds that much value.

@Hoverbear
Copy link
Collaborator Author

Hoverbear commented Nov 16, 2019

This is good feedback! I'm pretty lukewarm about the idea. While having a getter go unused is probably a bug, I'm not sure it's in scope here.

@SOF3
Copy link

SOF3 commented Nov 17, 2019

I don't see any reason why a getter would be unused though.

Except for rare debugging/testing reasons (like those in this pull request), but then those can be explicitly suppressed anyway.

@Razican
Copy link
Contributor

Razican commented Jan 15, 2020

If someone wants to lint for unused results (that don't have the #[must_use] attribute, they can lint on unused_results, so I think this should only be added for stuff that would probably break logic, as @djc suggested. Maybe it could be an optional attribute to the getters, but I don't think it's worth it to add it to all getters.

@Hoverbear
Copy link
Collaborator Author

Let's close it then.

@Hoverbear Hoverbear closed this Jan 15, 2020
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.

Generate #[must_use] for getters
4 participants