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

Pass Schedule by value and move #1942

Merged

Conversation

sweemer
Copy link
Contributor

@sweemer sweemer commented Apr 7, 2024

Schedule is a relatively heavy data structure, consisting of a couple vectors and other members, and is a prime candidate for moving instead of copying.

In fact several constructors already accept Schedule by value and move, so I am simply fixing the ones that aren't.

For the most part the changes are straightforward, but one thing to point out is that in overnightindexedswap.cpp I had to replace overnightSchedule with floatingSchedule() to access the moved-to member variable instead of the moved-from constructor arg, which may not be obvious from the variable names.

Another thing to point out, again in overnightindexedswap.cpp, is that the constructors taking a single Schedule argument end up copying twice - once into fixedSchedule and once into overnightSchedule. Perhaps these constructors should be deprecated in favor of the ones taking both schedules? I think that would help make the code more explicit and readable as well.

@coveralls
Copy link

Coverage Status

coverage: 72.497%. remained the same
when pulling bdbb8e1 on sweemer:pass-schedule-by-value-and-move
into 7c12452 on lballabio:master.

@lballabio lballabio added this to the Release 1.34 milestone Apr 8, 2024
@lballabio
Copy link
Owner

In the future we might try to reduce the number of constructors, but my reason would be to enable keyword args in Python :)

@lballabio lballabio merged commit daec722 into lballabio:master Apr 8, 2024
42 checks passed
@sweemer sweemer deleted the pass-schedule-by-value-and-move branch April 8, 2024 09:15
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.

3 participants