Replies: 4 comments 13 replies
-
|
Note that the no-op deleter can be the same for all types |
Beta Was this translation helpful? Give feedback.
-
Maybe a wild take on this, but don't you think it is worth breaking backwards compat in turn to simplify lifetime management? This should be done with a clear upgrade path described for users which want to go forward. From my understanding the "maybe owning" semantics stems from the issue that the objects are not reference counted in MFEM, hence it is not a-priori clear which object has to live for how long a priori. Hence there are flags introduced to manage this lifetime information manually. Keeping the product operator you have at top as an example, with shared pointer this problem would not occur, as the pointer itself manages its lifetime (by reference counting). Furthermore, for non-owning semantics we have https://en.cppreference.com/w/cpp/memory/weak_ptr |
Beta Was this translation helpful? Give feedback.
-
|
@v-dobrev and @pazner circling back to this topic, if we had a deprecation period then it seems like option 1 above would be the best medium term solution as we could "easily" support both the smart pointer and raw pointer APIs over a set of releases before deprecating the old API. During this time, we can point users to the new API using something like I'm bringing this up as I've been looking at implementing |
Beta Was this translation helpful? Give feedback.
-
|
@v-dobrev and @pazner for example this is how I'm starting to tackle it in one of my branches and outside of the warnings it just works with existing code or at least what I've tested so far: A few minor changes are needed to some constructors to reduce number of changes for the Also looks like some of my other small c++17 feature additions crept into that commit but overall the commit is focused on the smart pointer changes. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @rcarson3, this is useful. For reference, I have a WIP PR implementing option 2 here: #4629. I think it is largely compatible with your approach: you can convert back-and-forth between |
Beta Was this translation helpful? Give feedback.
-
|
If you're going through the work of switching to smart pointers, it would be good to reap the benefits and not have things that behave partially right. In the case of a |
Beta Was this translation helpful? Give feedback.
-
|
There is not a guarantee, only a "promise". The contract of the API is that if the user passes a non-owning Handle to another object, then the pointed-to object must have a longer lifetime. Keep in mind that in C++, neither |
Beta Was this translation helpful? Give feedback.
-
|
This is a follow up to my brief question on the slack #helpdesk channel. My opinion is that before tackling shared_ptr, you should utilize Similarly, some methods like ::std::unique_ptr<HypreParVector> GetTrueDofsU() const;You could gradually deprecate the raw pointer versions of those methods. The decision about how to implement shared pointers is difficult because there are various costs and benefits. The great thing about IMHO, you should also modify all the examples and miniapps to use unique_ptr's rather than new/delete. Programming by cut-n-paste is ubiquitous, so you examples should utilize best practices to encourage best practices in the community. |
Beta Was this translation helpful? Give feedback.
-
|
Overall I think something like this would be a step in the right direction. In situations where ownership is unambiguously unique, we should use In most of the examples, the |
Beta Was this translation helpful? Give feedback.
-
|
Is there an approach 3 where you introduce a new set of methods where clean |
Beta Was this translation helpful? Give feedback.
-
|
I'm not sure I understand the difference between approach 1 and what you call approach 3. Is it because of the "maybe owned" usage of The "maybe owned" semantic for |
Beta Was this translation helpful? Give feedback.
-
|
The semantics of shared_ptr is that the referenced object will remain as long as someone holds a reference to it. For example, I should be able to use a shared_ptr in a function registered with |
Beta Was this translation helpful? Give feedback.
-
|
The "not owning" |
Beta Was this translation helpful? Give feedback.
-
|
As an example, The idea with the "maybe owning semantics" is that the user has the option of taking responsibility for the lifetime of the object, and they promise that they will ensure that it lives at least as long as is required. Obviously this is more dangerous (you don't get the same guarantees that you do with reference counting), but it also makes the API much less restrictive. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
There has been an ongoing discussion on further incorporating smart pointers (in particular, shared/reference-counted pointers) in MFEM. In order to be backwards compatible, it seems required to model "maybe owning" semantics — the smart pointer may own the pointed-to object, or it may not, and this depends on runtime information. Perhaps the simplest example is something like
ProductOperator, whose declaration is given here:mfem/linalg/operator.hpp
Lines 892 to 909 in 899a96b
Depending on the boolean flags
ownAandownB, the resultingProductOperatormay or may not own the pointed-to operators. The current implementation has incorrect copy/move semantics, which may cause issues in otherwise well-formed code (e.g. storingProductOperatorin a standard container), and the manual management of ownership rules may cause subtle bugs/memory leaks (see e.g. #4613 from as recently as today).We can modify
ProductOperator(and similar classes) in a fully backwards-compatible way using smart pointers that model "maybe owning" semantics. We have discussed two approaches to achieve this:std::shared_ptr. An "owning"shared_ptris created as usual, whereas a "non-owning"shared_ptris created with a no-op deleter, e.g.[](T*){}. Convenience functions can be used to make specifying the ownership convenient and explicit.shared_ptr.shared_ptr(it was referred to in the meeting as an "abuse") and it may confuse users. We can't provide our own documentation for this class. An existingshared_ptrcan't be easily queried about whether it is owning or not.Handle<T>) that is built on top ofshared_ptr.14 votes ·
Beta Was this translation helpful? Give feedback.
All reactions