-
Notifications
You must be signed in to change notification settings - Fork 416
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
Simplify EquiZip
implementation
#902
Simplify EquiZip
implementation
#902
Conversation
NB: This may be a v4.0.0 PR? |
Codecov Report
@@ Coverage Diff @@
## master #902 +/- ##
==========================================
+ Coverage 92.41% 92.43% +0.02%
==========================================
Files 112 112
Lines 3426 3425 -1
Branches 1017 1019 +2
==========================================
Hits 3166 3166
+ Misses 199 198 -1
Partials 61 61
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Duplicate in the sense that both separate the implementations of the three zip functions, yes. However, #715 uses T4 to generate separate implementations of each method for argument count 2-4. Also, #715 and #902 have very different implementations. IMO #902 is simpler, because the control flow is more linear, rather than 4-way branch with multiple exit points. |
Some thoughts about this:
|
The various Zip* methods currently share implementation in
.ZipImpl()
. However, the code is simpler for each one when they are written separately; that is, in this case, the variances are more than the commonality. As such, we should allow each method to be implemented separately.This PR updates the implementation of
EquiZip
to be done separately from.ZipImpl()
.