search

fpagliughi / rust-industrial-io

Rust interface to the Linux Industrial I/O subsystem
MIT License
45 stars 21 forks source link

Added `#[must_use]` attribute, and const to functions where possible. Derivedd Eq besides PartialEq where possible. Removed structure name repition. #23

Closed 41Leahcim closed 1 year ago

41Leahcim commented 2 years ago

Added #[must_use] attribute, and const to functions where possible. Derivedd Eq besides PartialEq where possible. Removed structure name repition.

fpagliughi commented 2 years ago

Thanks for the PR! This crate doesn't get much love. I'll give this a test shortly; at the moment I'm busy on three other open-source projects.

I haven't implemented #[must_use] in any of my Rust projects, nor have I seen it used in most libraries, though it seems like std is starting to implement it. Do you see it becoming more common?

41Leahcim commented 2 years ago

I haven't seen #[must_use] used much either, but I like to use it in my own projects. I do think that it will become more popular in the future. It can help to decrease the number of function calls, by warning for unused returns of calls to functions without side effects.

fpagliughi commented 2 years ago

That sounds good. I'll give your PR a try momentarily.

It would be cool, though, if you could just enable it once at the library level rather than having to apply it to every function. (Meaning: set every function 'must_use' that does not have a unit return type).

I'll post a question on the Rust forum to help myself get a better feel for this.

Here https://users.rust-lang.org/t/liberal-application-of-must-use/85173

fpagliughi commented 2 years ago

Looking at the forum post ^^^, the overwhelming consensus is that #[must_use] should be applied somewhat sparingly. Have a read through the comments and the referenced When to add #[must_use] for the std library, and then let me know what you think, and we can come up with a solution.

But, to be clear, my only hesitation with this PR is the #[must_use] stuff. All the rest of it (const, Eq, Self, etc) looks awesome. I'd like to get that stuff in ASAP. Maybe use some of this to drive toward a new release in a few weeks.

41Leahcim commented 2 years ago

As I understand from that page of the Standard library developers Guide, #[must_use] should be used when code doesn't have side effects and the user might forget to use the returned value. From the forum post I understand, that it should only be used when not using the returned value has impact on performance. I would recommend to apply #[must_use] to large or complex types (like Device) and iterators (like AttrIterator), and remove it again from the functions.

fpagliughi commented 2 years ago

That seems a good compromise for now.

What I still don't understand is the intent for smaller, pure functions. The std library is still pretty inconsistent. The example from the Forum discussion is the len() and is_empty() functions that are #[must_use] for slices but not for Vec.

Perhaps as that shakes out we can use those guidelines to update this crate in the future.