-
Notifications
You must be signed in to change notification settings - Fork 902
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
Remove _copy_construct factory #8999
Remove _copy_construct factory #8999
Conversation
…ve_copy_construct
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #8999 +/- ##
===============================================
Coverage ? 10.58%
===============================================
Files ? 116
Lines ? 18634
Branches ? 0
===============================================
Hits ? 1972
Misses ? 16662
Partials ? 0 Continue to review full report at Codecov.
|
Looks really great to me overall! These changes are really good 😄 |
rerun tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@gpucibot merge |
This PR removes the
_copy_construct
factory method for theFrame
types that were still exposing it, replacing its usage with_from_data
. The current implementation of_from_data
is slightly faster, and more importantly this change leaves us with a single fast path for buildingFrame
objects (bypassing the slow constructors) so that we only have to maintain and optimize one going forward.