Skip to content

Implement more complete backend selection - #4118

Merged
rami3l merged 6 commits into
masterfrom
backends
Dec 20, 2024
Merged

Implement more complete backend selection#4118
rami3l merged 6 commits into
masterfrom
backends

Conversation

@djc

@djcdjc commented Dec 18, 2024

Copy link
Copy Markdown
Contributor

Feedback from #4105.

Comment threadsrc/utils/mod.rs
Comment threadsrc/utils/mod.rs Outdated
Comment threadsrc/utils/mod.rs Outdated
@djc
djcforce-pushed the backends branch 2 times, most recently from 2f3a3a5 to 3bfb0ffCompareDecember 18, 2024 10:56

@ChrisDentonChrisDenton left a comment

Copy link
Copy Markdown
Member

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.

@rami3lrami3l left a comment

Copy link
Copy Markdown
Member

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.

@ChrisDentonChrisDenton left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/utils/mod.rs Outdated
Comment threadsrc/utils/mod.rs Outdated
@rami3l

rami3l commented Dec 18, 2024

Copy link
Copy Markdown
Member

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

djc commented Dec 18, 2024

Copy link
Copy Markdown
ContributorAuthor

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

djc commented Dec 20, 2024

Copy link
Copy Markdown
ContributorAuthor

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.

@ChrisDentonChrisDenton left a comment

Copy link
Copy Markdown
Member

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 e63d552Dec 20, 2024
@rami3l
rami3l deleted the backends branch December 20, 2024 23:09
Sign up for freeto 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

@djc@rami3l@ChrisDenton@Dakota48423