search

rust-av / ffms2-rs

ffms2 bindings
MIT License
10 stars 5 forks source link

Refactor of struct creation macros and removal of custom getters/setters #19

Closed Christopher22 closed 3 years ago

Christopher22 commented 3 years ago

As the first important step to refactor the source code as mentioned in #16, this pull request removes the custom getters and setters in favor of Deref and DerefMut. This renders the exposed structures as smart pointers with additional convenience functions for creation and updating. By enforcing a consistent usage of create_struct! rather than its separate parts all over the crate and removal of the set_params! macro, the creation of structs is now unified and simplified.

Luni-4 commented 3 years ago

Thanks for your contribution! :)

Could you please split your commit in two commits?

  1. The first one containing the macros implementations
  2. The second one containing their replacement in the code

That subdivision would help me reviewing your changes

It seems there are two clippy lints that fail the CI, could you fix them?

Christopher22 commented 3 years ago

Hey Luni-4, you are welcome!

Originally, I planed to do exactly those two commits. However, there are quite entangled: Initially, the usage of create_struct! required providing all the tremendous parameters for set_params!, too. Stating those for the first comment just to remove them afterwards completely does not seem worth the work.

Sure, I will fix the clippy warnings.

Luni-4 commented 3 years ago

Originally, I planed to do exactly those two commits. However, there are quite entangled: Initially, the usage of create_struct! required providing all the tremendous parameters for set_params!, too. Stating those for the first comment just to remove them afterwards completely does not seem worth the work.

Ok, no problem then, I will try to review your PR the same.

Sure, I will fix the clippy warnings.

Great! I'm going to start reviewing as soon as the warnings are fixed

Christopher22 commented 3 years ago

Mmh. What's going on?! These enums have nothing to do with my changes, How did the warnings got into the master branch?

Luni-4 commented 3 years ago

That clippy warnings are suggesting to remove the pub(crate) fn [<to_ $func_name>](&self) -> $type macro since it isn't used anymore after your changes

Christopher22 commented 3 years ago

I assumed the would recommend removing the "to_audio_dither_method", "to_resample_filter_type", "to_matrix_encoding", and "to_mix_coefficient_type" implemented in terms of this macro. If your willing, I could remove this functions from the crate. But it is not any code I touched, ich just want to check twice.

Luni-4 commented 3 years ago

I assumed the would recommend removing the "to_audio_dither_method", "to_resample_filter_type", "to_matrix_encoding", and "to_mix_coefficient_type" implemented in terms of this macro. If your willing, I could remove this functions from the crate. But it is not any code I touched, ich just want to check twice.

For now we could add #[allow(dead_code)] for that macro and then verify the problem in another PR, in this way the clippy lint is ignored