Skip to content
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

Use copy_construct() instead of two set() call for rectangle assign() #13

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

plopresti
Copy link
Contributor

This produces significantly better code when the rectangle representation is an immutable pair of points (lower left / upper right); e.g. SSE vectors. And there is no cost in speed for the normal case, since modern compilers are very good at copy elision.

…gn().

This produces significantly better code when the rectangle representation
is an immutable pair of points (lower left / upper right); e.g. SSE
vectors. And there is no cost in speed for the normal case, since modern
compilers are very good at copy elision.
@plopresti
Copy link
Contributor Author

I realize my justification for this is basically "trust me". But trust me, this is a good change...

OK OK. What would you like to see for justification? Thanks.

@asydorchuk
Copy link
Member

Hi, I am on vacation right now. I am going to look at the changes on
Monday, July 18th.

On Jul 15, 2016 7:44 PM, "plopresti" [email protected] wrote:

I realize my justification for this is basically "trust me". But trust me,
this is a good change...

OK OK. What would you like to see for justification? Thanks.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#13 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABDj-ZXUqn-ktE0n16Y4cyksE_j4oS_hks5qV8cGgaJpZM4JI7Y4
.

@asydorchuk
Copy link
Member

Thanks for your contribution! However it doesn't seem that this pull request solves any problem. That's why I am reluctant to merge it, cause there is no active development on the library and any such small changes potentially can break the existing functionality.

@plopresti
Copy link
Contributor Author

I understand. Still, this makes a big performance difference in my application...

What if I create a patch that preserves the existing behavior by default, but allows me to override the implementation? (I am thinking of changing y_r_assign to a template class and having it default to "true"; then I can specialize it to provide a more efficient "assign" function for my own class.)

If this sounds vaguely acceptable, I will work up a pull request. Thanks.

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.

2 participants