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

Deprecate RelinkableHandle constructor taking a raw pointer #1978

Merged
merged 2 commits into from
May 30, 2024

Conversation

sweemer
Copy link
Contributor

@sweemer sweemer commented May 23, 2024

I'm getting 'QuantLib::Handle<T>::Handle': no overloaded function could convert all the argument types when trying to use the constructor of RelinkableHandle that takes a raw pointer. Not sure if I'm doing it right, but even if it does work, I believe that this constructor should be deprecated for the following reasons:

  • Handle does not provide a constructor taking a raw pointer and it's unclear why RelinkableHandle needs to.
  • shared_ptr takes ownership of the raw pointer, which may not be obvious from the constructor declaration, and can lead to double delete bugs.

Also I changed a couple of places to pass by value and move.

@coveralls
Copy link

coveralls commented May 23, 2024

Coverage Status

coverage: 72.55% (-0.001%) from 72.551%
when pulling a19e2ec on sweemer:relinkable-handle
into b981510 on lballabio:master.

@sweemer
Copy link
Contributor Author

sweemer commented May 28, 2024

@lballabio Do you want me to revert these back to const reference too? Not sure whether you think that the case we want to optimize for is the same between shared_ptr and Handle.

@lballabio
Copy link
Owner

Hmm. We're dealing with just Handle and RelinkableHandle here, not a bazillion indexes, so we might consider overloading on const T& and T&&. What do you think?

@sweemer
Copy link
Contributor Author

sweemer commented May 29, 2024

Sounds good. I'll add those for both Handle and RelinkableHandle.

@sweemer
Copy link
Contributor Author

sweemer commented May 29, 2024

I added a default constructor for RelinkableHandle to disambiguate between the two new constructors, and also changed linkTo to pass by value, both of which are now in line with Handle.

@lballabio lballabio added this to the Release 1.35 milestone May 30, 2024
@lballabio lballabio merged commit 4afdf27 into lballabio:master May 30, 2024
42 checks passed
@sweemer sweemer deleted the relinkable-handle branch May 30, 2024 20:46
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