raymanfx / libv4l-rs

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

Use struct initializers instead of member initializers over zeroed data #53

Closed MarijnS95 closed 2 years ago

MarijnS95 commented 2 years ago

The main advantages are:

Downsides:

Disclaimer: I wrote these changes some time ago, and haven't revisited the entire diff to make sure everything is correct. Self-review pending... :)

MarijnS95 commented 2 years ago

@raymanfx Perhaps my views are biased exactly for working with Rust every day, and leaving C++ behind since a few months.

However, still working with C on a regular basis in the Linux kernel where struct initializers are also desired (and the only way to write const static XXX initializers) :wink:

raymanfx commented 2 years ago

.. I don't know which parts of the kernel you are working on, but I generally don't get to enjoy the luxury of nice and clean code in the drivers I see :(

One small request: could you please rebase this PR against the next branch? I have several other improvements lined up there and would like to get a common baseline before merging it back into master and minting a new release.

MarijnS95 commented 2 years ago

.. I don't know which parts of the kernel you are working on, but I generally don't get to enjoy the luxury of nice and clean code in the drivers I see :(

Mostly qcom drivers and DTs, though lately more on the side of review of downstream-copy-paste drivers of terrible quality :sob:


Rebased and target branch updated, that's pretty trivial :wink:

MarijnS95 commented 2 years ago

Whoops, I missed that extra import amidst a plethora of other lints. Expect a fix for those soon, after this PR :)

raymanfx commented 2 years ago

Yeah, this is just one that came up when running cargo check, I did not hunt for the myriad of clippy lints that are easy targets to pick. Are you fine with me landing the PR once CI is done?

MarijnS95 commented 2 years ago

@raymanfx Yes please, after double-checking the remaining code above. I replicated some refactoring done to mmap/stream.rs to {io,mmap}/{arena,stream}.rs for consistency, everything else looks fine to me.

EDIT: Also squashed down to save noise in the final commit message (though you probably curate that already).

raymanfx commented 2 years ago

Alright, so let me know once this is good to go and we'll land it.

MarijnS95 commented 2 years ago

Yeah, it was good after the last push - if you agree with the extra changes, of course :)