rust-bindgen icon indicating copy to clipboard operation
rust-bindgen copied to clipboard

Split `BindgenOptions`

Open pvdrz opened this issue 3 years ago • 3 comments

This PR splits all the fields inside BindgenOptions struct into two new structs:

  • inputs: BindgenInputs: Which holds all the command line arguments and implements Clone.
  • state: BindgenState: Which holds the remaining fields of BindgenOptions and does not implement Clone.

This change would allow bindgen to preserve its initial configuration and run again if required (which would help solve #1090).

It would also ease a transition to a newer version of clap (https://github.com/rust-lang/rust-bindgen/issues/1873) and is somewhat related to https://github.com/rust-lang/rust-bindgen/issues/2132.

Additionally BindgenOptions::inputs is exposed immutably, meaning that it is not possible to accidentally change the user inputs.

cc @emilio @amanjeev

Blocked by: https://github.com/rust-lang/rust-bindgen/pull/2271

pvdrz avatar Sep 05 '22 21:09 pvdrz

:umbrella: The latest upstream changes (presumably 8b29355ca0ce54e941d398ef9a605e9b5c0f20ae) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Sep 10 '22 01:09 bors-servo

:umbrella: The latest upstream changes (presumably 61636e94ca315278d9ac5f1210ffca6cca697428) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Sep 11 '22 13:09 bors-servo

:umbrella: The latest upstream changes (presumably 0d805d70d5c6e4cc99cbad7a988c38bdfe52321b) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Sep 19 '22 00:09 bors-servo

:umbrella: The latest upstream changes (presumably 86f059f081160ba01de31fc2ec6e20e52b97968d) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Sep 23 '22 06:09 bors-servo

I agree that it would be possible to preserve the user inputs by just cloning BindgenOptions before calling generate. But at the same time, I believe this change makes clearer when a value was set by the user with some intent vs when bindgen modified one value due to its internal logic.

At the same time, keeping all the CLI inputs in a single type makes easier moving to things like clap 3 with its derive macro. Which would ease adding new flags and keeping the documentation and names consistent as they would be a bunch of field attributes instead of being on a different file.

I'm not opposed to just cloning things before they are changed. I just think this makes the code a bit more organized while only adding the extra burden of thinking if something is part of the inputs or part of the state to decide where to find it. At the end most of the changes done to the uses of BindgenOptions were reduced to doing

+ foo.options()
- foo.inputs()

pvdrz avatar Sep 23 '22 16:09 pvdrz

:umbrella: The latest upstream changes (presumably #2278) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Sep 24 '22 02:09 bors-servo

Closed in favor of #2285

pvdrz avatar Sep 26 '22 18:09 pvdrz

Sad to this closed, seemed like a good code quality improvement

divagant-martian avatar Sep 28 '22 19:09 divagant-martian