raymanfx / libv4l-rs

Video4Linux2 bindings for Rust
MIT License
155 stars 65 forks source link

make Format::new const #20

Closed sigmaSd closed 3 years ago

sigmaSd commented 3 years ago

https://github.com/raymanfx/libv4l-rs/issues/17

sigmaSd commented 3 years ago

There are also other functions in the code base that could be const.

Should const be used everywhere its possible or there are some drawbacks?

sigmaSd commented 3 years ago

One possible drawback is that changing from const to not const is a breaking change.

raymanfx commented 3 years ago

What other functions do you have in mind? As long as it solves a problem or represents a valid usecase, I don't see why we can't make these functions const.

sigmaSd commented 3 years ago

You can use clippy to check:

cargo clippy -- -D clippy::missing_const_for_fn
warning: unused manifest key: workspaces
    Checking v4l v0.10.2 (/home/mrcool/dev/others/libv4l-rs)
error: this could be a `const fn`
   --> src/device.rs:261:5
    |
261 | /     pub fn fd(&self) -> std::os::raw::c_int {
262 | |         self.fd
263 | |     }
    | |_____^
    |
    = note: requested on the command line with `-D clippy::missing-const-for-fn`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn

error: this could be a `const fn`
  --> src/format/fourcc.rs:23:5
   |
23 | /     pub fn new(repr: &[u8; 4]) -> FourCC {
24 | |         FourCC { repr: *repr }
25 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn

error: this could be a `const fn`
  --> src/fraction.rs:25:5
   |
25 | /     pub fn new(num: u32, denom: u32) -> Self {
26 | |         Fraction {
27 | |             numerator: num,
28 | |             denominator: denom,
29 | |         }
30 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn

error: this could be a `const fn`
  --> src/timestamp.rs:26:5
   |
26 | /     pub fn new(sec: time_t, usec: time_t) -> Self {
27 | |         Timestamp { sec, usec }
28 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn

I don't have a use-case in mind, this is just an observation.