Skip to content

Implement more complete backend selection #4118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 20, 2024
Merged

Implement more complete backend selection #4118

merged 6 commits into from
Dec 20, 2024

Conversation

djc
Copy link
Contributor

@djc djc commented Dec 18, 2024

Feedback from #4105.

@djc djc force-pushed the backends branch 2 times, most recently from 2f3a3a5 to 3bfb0ff Compare December 18, 2024 10:56
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is better now but still a bit less clear then I'd like. Maybe that's unavoidable. Perhaps using cfg! instead of #[cfg] would help but perhaps not.

As it is, I'd be ok approving this but I'd want to hear @rami3l's thoughts first.

Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djc Thanks for your efforts!

I do think the new version looks much better for two reasons:

  1. It's clear why each conditional branch is doing certain things (and thus making the review easier).
  2. If I'd like remove a feature flag (I hope this won't be too far in the future), I can easily figure out what parts of the code to rip off.

@ChrisDenton We have 7 cfgs (2 ^ 3 backend stacks - 1 won't build, if I'm not mistaken) to consider in this piece of code, so unfortunately it will be very long if it was made absolutely plain. My guess is that we'll start to deprecate backends shortly, so this seems bearable.

Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 7 cfgs (2 ^ 3 backend stacks - 1 won't build, if I'm not mistaken) to consider in this piece of code, so unfortunately it will be very long if it was made absolutely plain. My guess is that we'll start to deprecate backends shortly, so this seems bearable.

That's fair.

@rami3l
Copy link
Member

rami3l commented Dec 18, 2024

Also did a bit more work to make sure that the match expressions are exhaustive in most combinations of the Cargo features.

I think this is better now but still a bit less clear then I'd like. Maybe that's unavoidable. Perhaps using cfg! instead of #[cfg] would help but perhaps not.

@djc I think this is where cfg_if shines with its else conditions. I know you had avoided using it and I don't know if it will work at all here, but maybe you can give it a shot?

@djc
Copy link
Contributor Author

djc commented Dec 18, 2024

I think this is better now but still a bit less clear then I'd like. Maybe that's unavoidable. Perhaps using cfg! instead of #[cfg] would help but perhaps not.

@djc I think this is where cfg_if shines with its else conditions. I know you had avoided using it and I don't know if it will work at all here, but maybe you can give it a shot?

I agree that this might not be the optimal encoding but I'm also inclined to spend a bunch more time on it -- this takes care of covering all the cases with reasonably good feedback for the user, and I think that's a good improvement over the status quo. For me, the code isn't bad enough that it's worth investing much more in.

@djc
Copy link
Contributor Author

djc commented Dec 20, 2024

So brought this back to a single match (for the exhaustiveness checking) and checked compilation under most of the potentially relevant feature combinations -- there are now 8 arms in 3 groups which I think makes them easier to reason about? Also cleaned up the download crate a little to get more compile-time errors under different feature combinations.

Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this version and in any case I agree it's not worth spending more time on tbh.

@rami3l rami3l added this pull request to the merge queue Dec 20, 2024
Merged via the queue into master with commit e63d552 Dec 20, 2024
27 checks passed
@rami3l rami3l deleted the backends branch December 20, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants