I am changing the class Document to use std::shared_ptr to manage pointers to DocumentObject. Basically, I want to change DocumentP::objectMap to hold a std::shared_ptr instead of a raw pointer. I am doing this, because I am developing a replacement for ObjectIdentifier that uses shared_ptr.
Everything is very simple, except the “Transaction” (undo/redo infrastructure). I don’t fully understand how and when the Transaction class assumes ownership of a DocumentObject*.
For example, Transaction::addObjectNew takes a TransactionalObject pointer… that is, a DocumentObject* or a ViewProvider*. I assume that when it takes ownership of a pointer, Transaction stores it in “_Objects”. So, I guess I have to change this “map” and make it store a std::shared_ptr instead of a raw pointer.
Now, because we are not dealing with DocumentObject, but with the parent class TransactionalObject, this change would affect, for example, any ViewProvider. I see that, like DocumentObject* is managed by App/Document.h, the file Gui/Document.h is responsible for keeping a list of ViewProviderObject*. I am assuming I would have to change this to become a shared_ptr, instead.
I would love to know:
What lists of pointers to descendants of TransactionalObject do I have to worry about? App:Document::d->objectMap. Gui::Document::d->_ViewProviderMap. Transaction::_Objects. Anything else?
Do you have any recommendations or comments you would like to share with me?
Does the Transaction class, in special Transaction::_Objects, happen to hold anything besides DocumentObject* and ViewProviderObject*?
Using a std::shared_ptr outside a Document is a very bad idea because this way the class loses the ownership of its objects and it’s impossible to guarantee that if the Document deletes an object that it is really deleted because any other instances can keep a reference and prevents it from being deleted.
If someone wants to keep a reference… who am I to judge!
Sorry for the joke. But this worry that “some one will hold a reference” past the supposed life of the object is a little silly if you compare to holding a raw pointer.
If you can assume no one holds a pointer to a deleted object you can as well assume no one holds a shared_ptr to something that should be released. In my opinion, holding a raw pointer to some destroyed object HAS TO BE CONSIDERED A BUG. Even if you don’t access the pointer… it won’t crash… but it is still a bug.
As far as I understood, FreeCAD has a signaling system to inform when the objects are removed. At this point, all you have to do is release the shared_ptr (if you think you should). In the case of my “Accessor” scheme, all you have to do is “re-resolve” the reference to get a new shared_ptr or a “could not resolve” exception.
A memory leak would show you the bug when holding a raw pointer would not show anything.
The discussion on the overhead of shared_ptr is totally irrelevant to a piece of software that already has a huge overhead when allocating an object. When you create a DocumentObject, you pick up a unique name, an id, stores the information in a bunch of maps and a vector. An atomic increment is a drop in the ocean, here.
A shared_ptr can be passed by reference and can also be moved. You can also pass the pointer or a reference to the DocumentObject as long as you know the function will not hold the reference past its return.
And even if you do copy those shared_ptr around (I wonder why you would do that)… the guy took 4 seconds to copy one million of shared_ptr!!!
You have striped down every useful feature of a weak_ptr. You are reinventing the wheel in a simplistic way. There are no warranties of anything when you use this class to reference a pointer. It seems this just encapsulates the signaling mechanism. And it is totally thread unsafe. (yes, you can find posts saying shared_ptr is not thread safe… but again, what is thread safe and what is not needs to be fully understood)
This DocumentObjectWeakPtr is certainly slower then a shared_ptr or a real weak_ptr, and it is not as useful.
I see a near future where FreeCAD will be multi-threaded in different ways. This would be a real gain. You would be sure that even if the object were removed from the tree… your other thread using it to generate a “Shape” would not need to worry! Of course, it would have to update the shape, later.
If you want to use a real std::weak_ptr, I am fine with it. (I would not recommend, because I think the design of this mechanism should be “brain stormed” before a decision based on too generic arguments.) But in this case, the objectMap would have to hold a shared_ptr.
If you can assume no one holds a pointer to a deleted object you can as well assume no one holds a shared_ptr to something that should be released.
I disagree. Making an API based on assumptions is the wrong way.
In case the Document class were changed to hold shared pointers and it exposed the shared pointers it cannot guarantee any more that the object will be deleted when it should be. And because the Document will lose control and at the same time we cannot rely on a programmer to destroy the instance that holds the shared pointer there is a rather high risk that we have memory leaks.
In some modules (like FEM or Mesh) the data can be quite big so that due to a careless programmer we may have leaking memory in the realm of GB.
So, if we decide to change the current implementation then only by exposing a weak pointer to the calling instance.
In my opinion, holding a raw pointer to some destroyed object HAS TO BE CONSIDERED A BUG.
Therefore we have the possibility to observe a document. So, a class that holds a raw pointer that can be deleted during its lifetime must implement a function to observe the object it holds.
A memory leak would show you the bug when holding a raw pointer would not show anything.
How is this going to work? Unless you have a really big memory leak you probably will never notice it. At least a dangling raw pointer that causes a segmentation fault is much easier to detect.
The discussion on the overhead of shared_ptr is totally irrelevant to a piece of software that already has a huge overhead when allocating an object. When you create a DocumentObject, you pick up a unique name, an id, stores the information in a bunch of maps and a vector. An atomic increment is a drop in the ocean, here.
I have written a little test application to see the differences between raw and shared pointer. There is a little overhead in creating a shared pointer but this is negligible as the extra time is around a microsecond for an object. More relevant is the access to an object during its lifetime because objects are accessed all the time. Compared to a raw pointer the access of a shared pointer is 3x slower (with optimization enabled it’s still 2x). However, the absolute measured times for 1000 objects and accessed 1000 times is in the realm of a few microseconds. So, in the end the extra overhead of shared pointers are negligible.
You have striped down every useful feature of a weak_ptr. You are reinventing the wheel in a simplistic way.
It doesn’t reinvent anything. All what it does to offer a similar API as a std::weak_ptr.
And it is totally thread unsafe.
The whole Document framework is not thread-safe and there are no attempts to do so because it simply doesn’t make sense. In more than 90% when doing a recompute then there is a strict sequential relationship of the involved objects so that there is no chance to parallelize anything.
Also an ex-OCC developer who appears from time to time in the forum clearly stated that multi-threading in the area of CAD is pointless.
This DocumentObjectWeakPtr is certainly slower then a shared_ptr or a real weak_ptr, and it is not as useful.
It’s very likely slower but usually it’s temporarily used for a single object only. So, its extra cost is totally irrelevant.
I see a near future where FreeCAD will be multi-threaded in different ways. This would be a real gain.
Forget it! This will never work on high-level stuff like the document framework because any change on a document object sooner or later will trigger a change of a Qt widget. But because Qt itself is not thread-safe you had to use mutexes everywhere and dramatically slow-down the application.
The only chance I see for multi-threading is for low level geometries, e.g. on OCC shapes or meshes.
This is a bit shallow. Every time you do something, you do it based on assumptions. Also, I am not saying you should do any assumptions, here. The statement is very precise:
IF you can assume “no one holds a pointer”, THEN you can assume “no one holds a shared_ptr”.
I am not saying in this exact sentence that you can assume anything. But yes, it seems to me that we “kind of” already assume no one holds a pointer to an invalid document object. IF this assumption is true, THEN “no one holds a shared_ptr to something that should(??) be destroyed” is ALSO true. But, only IF.
The definition of “should be deleted” is very complicated. The promiscuous relation between Document and Transaction show that. There is a much more precise and much safer way to define “should be deleted”. It should be deleted when the last share was released. This is the whole purpose of a shared_ptr. But I do understand people being afraid of it going “out of control”.
There are some alternatives (I am not advocating for any, right now):
Do nothing.
Hold a shared_ptr “internally” and expose a weak_ptr.
Hold a shared_ptr “internally” and expose the raw pointer.
Hold a shared_ptr “internally” and expose a weak_ptr and a shared_ptr.
== Item 3 ==
Item 3 could be implemented right now. It would simply make the management of the responsibility easier. For example, there would be no need for the Document to know if the object is being held by a Transaction or not. The Document releases its “share” when it does not need it anymore. And the Transaction releases it when it does not need it anymore. The object will be destroyed when no one holds a shared_ptr.
== Item 2 ==
Item 2 is an extension of item 3 that would allow for a “check” mechanism that does not depend on signaling. It is much safer. Programmers would be told (through documentation) to hold a lock only temporarily, in local variables. They would be forbidden (by documentation) to hold the lock out of the temporary scope.
A bad thing about this is that every time one uses the weak_ptr, there would be some “boiler plate code” to hold a lock… and the failure to do so would have to be dealt with. AGAIN, if you instead hold a raw pointer and simply assumes it is valid… there should be no failure… but a locked_ref = freecad_lock_object(my_weak_ptr) could throw a standard exception, just to be sure.
== Item 4 ==
The difference of item 4 and item 2 is basically the documentation. You would not be forbidden to keep a shared_ptr. The API would be easier to use because you could keep a shared_ptr, directly. So things would not have to be divided in two steps: acquire the lock and use it.
For item 4, however, the programmer is not supposed to hold a shared_ptr to some object past its “supposed existence”. The current signaling mechanism used for raw pointers could be used to warrant this. With the difference that when receiving the signal, the destroyed object can still be referenced… things could be made before the shared_ptr is released.
It is important to notice that item 4 does not necessarily mean that everybody will be holding a shared_ptr. Probably, ObjectIdentifier could encapsulate this and set up the proper signals and actions. This would be easier then using weak_ptr because there are less exceptions to deal with. ObjectIdentifier could even have a “cache policy” that would decide what to do upon object destruction. The warranty that the shared_ptr will be properly released is basically the same warranty that the invalid pointer will not be referenced anymore.
Yes… loosing control… is terrifying!!!
You have to call it a “lock” or a “locked_reference” and get the programmer to destroy the thing. I want to suppose that the programmer is responsible enough for not holding a “locked_xxx” indefinitely.
Some years ago (and even today) you could find lots of people that would tell you that a system like “git” is not good/possible because you “loose control” of what files your colleague is editing. They would argue:
How can I be sure they will not mess with my file? How can I lock the file so I will not loose control?..
I am very happy with this being considered. I do believe that a deep discussion could lead to the conclusion that a well thought design could turn out to be more in favor of a shared_ptr. And, in fact, when you expose a weak_ptr, you do expose a shared_ptr. What you can do is, through documentation, kindly ask the programmer to not hold the locked resource.
Exactly! That is my point!!!
If you can do this for a raw pointer, you can do this as well for a shared_ptr. It is just the same.
You get the signal and you “release the pointer”. With the advantage that you can still access it before it gets destructed.
Option 1: memory leak you will never notice.
Option 2: crash.
But I don’t really think this is a useful discussion. So, I withdraw my statement.
Sorry. I fail to see that. It does sets up the signals… which is great!
In particular, weak_ptr has a method that should seldom be used: “expired()”. It does work to make sure the thing has indeed expired. But it should NEVER be used to make sure the pointer has not expired. Using “expired” this way is wrong. What weak_ptr does for you is allowing you to lock the resource while you need it. You don’t need any additional hypothesis (assumptions) on how the thing works… if it is or not multi-threaded… if it is or not on a Transition… if it is or not in a different Document… if someone forgot or not to emit a signal…
It is a raw pointer wrapped in signals and with some “expiration” check. It does what you should not be doing if you were using a weak_ptr.
But I don’t think this discussion is useful, either. I am sorry I have criticized “DocumentObserver”.
IF you can assume “no one holds a pointer”, THEN you can assume “no one holds a shared_ptr”.
No, you can’t. There are scenarios where an instance holds a raw pointer and by deleting the object it doesn’t matter that the pointer becomes dangling because the program logic makes sure that the pointer won’t be accessed any more.
By holding a shared pointer it’s guaranteed that it’s always in a well-defined state. But problems appear as soon as for some reason the instance that holds the shared pointer isn’t destroyed any more and thus holding the shared pointer forever. Especially as soon as things are exposed to Python it easily happens that the Python object isn’t destroyed any more due to cyclic references, a forgotten decrement of the reference counter and so on. So, the shared pointer won’t be released either. Even high-level libraries like pybind11 have problems with leaking memory.
Hold a shared_ptr “internally” and expose the raw pointer.
Makes actually no sense to me because in the best case it doesn’t change anything and in the worst case it could lead to unexpected dangling pointers. Not every programmer is aware of the fact that once a raw pointer is used it must not put into a new shared pointer because then the new shared pointer will destroy it and the original shared pointer will have a dangling pointer.
Item 3 could be implemented right now. It would simply make the management of the responsibility easier. For example, there would be no need for the Document to know if the object is being held by a Transaction or not. The Document releases its “share” when it does not need it anymore. And the Transaction releases it when it does not need it anymore. The object will be destroyed when no one holds a shared_ptr.
q.e.d.
== Item 2 ==
Item 2 is an extension of item 3 that would allow for a “check” mechanism that does not depend on signaling. It is much safer. Programmers would be told (through documentation) to hold a lock only temporarily, in local variables. They would be forbidden (by documentation) to hold the lock out of the temporary scope.
I agree with this.
A bad thing about this is that every time one uses the weak_ptr, there would be some “boiler plate code” to hold a lock… and the failure to do so would have to be dealt with. AGAIN, if you instead hold a raw pointer and simply assumes it is valid… there should be no failure… but a locked_ref = freecad_lock_object(my_weak_ptr) could throw a standard exception, just to be sure.
Using a weak pointer has a little more overhead than using a shared pointer only but this is so small that it really doesn’t matter in that context.
For item 4, however, the programmer is not supposed to hold a shared_ptr to some object past its “supposed existence”. The current signaling mechanism used for raw pointers could be used to warrant this. With the difference that when receiving the signal, the destroyed object can still be referenced… things could be made before the shared_ptr is released.
For this I only see a valid use case for transactions because when removing an object from the document a transaction could become the new owner. But this can also be implemented differently.
The most viable alternatives to me are 1) and 2).
What you can do is, through documentation, kindly ask the programmer to not hold the locked resource.
This is wishful thinking. Even the best programmer runs into a situation where he cannot guarantee any more that a resource will be freed. If everything was done in C++ only there is a fair chance to avoid this but as soon as things are exposed to Python this will become very, very difficult.
Sorry. I fail to see that. It does sets up the signals… which is great!
In the past if a programmer had to care about nullifying a raw pointer he had to write a lot of code to observe what happens to a document. The DocumentObjectWeakPtrT/WeakPtrT encapsulates all the ugly stuff and makes it very convenient to be notified about the destruction of the object.
But it should NEVER be used to make sure the pointer has not expired. Using “expired” this way is wrong.
Who says this and why?
What weak_ptr does for you is allowing you to lock the resource while you need it.
Switching to smart pointers, particularly std::shared_ptr, is a good modern C++ approach that ensures better memory management and eliminates many risks associated with raw pointers. However, there are nuances to consider, especially in a codebase where ownership semantics are essential.
Lists of pointers to descendants of TransactionalObject:
You’ve mentioned App::Document::d->objectMap, Gui::Document::d->_ViewProviderMap, and Transaction::_Objects. These are primary lists where ownership matters, especially for undo/redo functionality. It would be beneficial to grep or search through the entire codebase for any references to TransactionalObject or its descendants to ensure you haven’t missed any.
Check the API documentation or any architectural diagrams if available, which can sometimes highlight major relationships between classes.
Recommendations or comments:
Before making the transition, ensure you understand the ownership model. Are there places where multiple parts of the codebase believe they have ownership? If so, std::shared_ptr is appropriate, but if ownership is transferred, consider std::unique_ptr.
Carefully manage the transition from raw pointers to shared pointers. In places where you’re unsure about the ownership, leave comments and TODOs so that you or other developers can revisit them.
Keep in mind the slight overhead of std::shared_ptr because of its reference counting. It’s typically negligible, but in performance-critical areas, it’s worth considering.
Ensure you refactor incrementally and test the functionality often. Changing the memory management model can introduce subtle bugs.
Contents of Transaction::_Objects:
Transaction::_Objects seems to deal with both DocumentObject* and ViewProviderObject* as you’ve noted. Without seeing the full source code, it’s hard to definitively state if other types are stored. To ensure that you capture everything, you might want to add some static asserts or runtime checks to ensure that all stored objects are indeed of types you’re considering for the shared_ptr transition.
You might also need to dig deeper into any polymorphic behavior associated with DocumentObject and ViewProviderObject, especially if other classes inherit from them and can potentially be stored in Transaction::_Objects.
Remember, the transition to std::shared_ptr should simplify your code and make ownership more transparent. If you find things becoming more complicated, it’s worth stepping back and reassessing. Good luck with your refactoring !
Please, feel free (if you want) to ignore (any of) my comments. I think we both understand each other already, and I don’t want to take your time by discussing things that might be too far from being useful. <3
A function or a piece of program is, in a sense, a theorem. You pass it variables (a state) and you think it does something with it, taking the thing to a new state. When you read the code, you are reading a demonstration that it in fact does what it is supposed to. Good code needs to be easy for anyone who reads it to conclude that the code is correct. Many different rules for good design are a form of this statement. When you say “global variables are bad”, he is saying that “if you have global variables it is more difficult to know if the program is correct”. When you say “const correctness”, you are saying that the more predictable, the easier to demonstrate the program is correct.
When you say that at first sight the program looks incorrect, but if you follow the “logic” you can conclude, usually from implementation details, by following some very complicated path and using lots of assumptions… that the program is in fact “correct”… this scares me a little bit. This should trigger some “read alert” and the design should at least be rethought.
Yes, python has inherent memory leak when the programmer produces cyclic references. I don’t think that producing a dangling pointer is a good response to this.
Nonetheless, you are still allowed to use pointers even when you have a shared_ptr. From the current state of FreeCAD, I would never suggest to change everything to shared_ptr. You can still have functions that take a raw pointer and even stores it somewhere. A shared_ptr (and the weak_ptr) is a form of communication. If you don’t want to communicate and simply assume things are okay… you can.
Well… FreeCAD’s “internally” is a huge amount of code. The relation between Transaction and Document… or even Document and Document in case of a “move”… is an example of how complicated ownership is.
This programmer you mention is very very lost.
His code will surely crash. I just hope he takes the opportunity to learn instead of doing some hackish stuff to avoid the “crash”.
If you pass a raw pointer/reference to a function, it is fine. The pointer can be assumed to stay valid during the function call. You are NOT passing ownership.
If you pass or return a unique_ptr, you are passing the ownership. If you pass a shared_ptr, you are sharing ownership. And there is no way back.
I don’t care for the “processing overhead”. I am just mentioning that when you use the weak_ptr, you need to lock it, to convert it to a shared_ptr. And you have to check if the locking has or has not succeeded. And you have to deal with it.
The weak_ptr is a form of communication. It is like a delayed way to inform the object has been destroyed. After you lock it and the lock succeeds, you have the warranty that it will not be destroyed (except for that programmer above ). Ideally, the lock is a local variable… it will be released when it gets out of scope.
I just wanted to mention that there is some boiler plate code. But this code could be wrapped inside some DocumentObserver. Or inside ObjectIdentifier. Or… inside my future new Accessor.
Yes. I should have written:
What you can do is, through documentation, kindly ask the programmer to not hold the locked resource, AND PRAY.
But that is what you are doing when you pass a weak_ptr. You HOPE that the programmer will take this weak_ptr, and only lock it in the local scope of some function. But there is nothing you can do if the guy insists in taking the weak_ptr and storing the lock (that is, a shared_ptr).
The idea behind weak_ptr is this:
You have a weak_ptr “wp” to a resource that might or might not be destroyed.
When you want to access the resource, you call auto sp = wp.lock() to get a shared_ptr version of it.
If the returned lock (shared_ptr) is null (evaluates to boolean false), the resource is not available anymore.
If the returned lock is not null, the resource is warrantied to be valid while this lock exists.
When “expired” evaluates to false, it might be that on the next tick it has already expired. Of course… if you want very much to assume no other thread will ever release the resource while you are accessing it (and I see no reason to do so, even if it is true today), then you can use “expired()” and use your weak_ptr in a way it was not designed to be used. And worse… you would be teaching people to do the wrong thing.
Yes. This is the whole idea behind a weak_ptr. It is a pointer that can become invalid. Before using it, you have to check it is valid. While you hold a valid lock, you are assured it will be kept valid.
Instead of calling “if(wp.expired())”, you first acquire a lock and then you test the lock. If it is valid, you can use the lock to access the resource, because the lock is actually a valid shared_ptr to the resource.
This is the boiler plate code I was talking about.
When I say I want, for example, App::Document::d->objectMap to hold a shared_ptr, I do not mean that every other DocumentObject needs to be handled by a shared_ptr. Only those in App::Document::d->objectMap.
But I understand you! The question is:
Can I assume every pointer in App::Document::d->objectMap is owned by this Document instance?
Those performance-critical things can simply keep using raw pointers. Actually, most of the functions in FreeCAD can (and should) simply keep using raw pointers or references. When you pass a raw pointer to a function you are saying that you are sure the pointer will remain valid until the function returns.
Yes… what a nightmare!
Good. I love “asserts”.
I think the code can become more complicated when you have to take care not to destroy the last instance of a certain shared_ptr. But this situation also enlightens the whole thing a little.
Please, feel free (if you want) to ignore (any of) my comments.
I think I can agree with almost everything you have written below. Just a few remarks to a few points…
If you > pass > or return a unique_ptr, you are passing the ownership.
Passing a unique_ptr (by value) to a function is not possible because the copy constructor is disabled. So, there is no way to implicitly transfer ownership of a unique_ptr. This is on purpose because that was a serious problem with the deprecated auto_ptr in C++98.
But that is what you are doing when you pass a weak_ptr. You HOPE that the programmer will take this weak_ptr, and only lock it in the local scope of some function. But there is nothing you can do if the guy insists in taking the weak_ptr and storing the lock (that is, a shared_ptr).
Of course there is no way to make absolute guarantees. But my idea is to make some harder restrictions via the API so that it becomes harder to make things wrong.
The idea behind weak_ptr is this:…
You mean the possible race condition between the calls of expired() and lock(). Yes, that’s correct.
That is fantastic about C++… the copy and copy constructor is disabled! So… you will not violate the uniqueness.
You have to pass it using std::move(ptr_i_will_not_own_anymore).
By the way, I was getting a warning on some recent change… I think in the Sketcher… I don’t remember… But there, you create a local variable and return it using
return std::move(local_var);
According to the warning, you should not (need not) use std::move because when you return a local variable, it is already movable.
Yes. I understand and respect that.
I think it would be nice to have a discussion where:
The problem is well stated.
A proposed solution is precisely formulated.
The fact that the proposed solution does in fact resolves the problem is demonstrated.
Other options are evaluated and compared.
Use cases are tested.
Maybe ObjectIdentifier could hold a weak_ptr. Then it could implement a function like “getLockedPointer()” that would:
Return a valid shared_ptr; or
Try to re-resolve the reference and return a valid shared_ptr.
In case it does not resolve, do whatever it usually does in this case… throw exception, etc.
Yes. It simply makes no sense to use expired() before a lock(). You simply lock() and that’s it. I believe that any code that uses “expired()” to make sure the thing has not expired, is badly designed.
Imagine the scenario where you have raw pointers spread along the code. Then, you use weak_ptr::expired() to make sure those pointers are valid. Then you call functions that use the supposedly valid raw pointers. That would be a very wrong usage of “expired()”. But it would only cause problems in race conditions. You would be assuming there aren’t any.
I was actually I was basically talking about FreeCAD’s simplified weak pointer. My point was that it has an expired() method that is probably used to make sure the raw pointer is still valid. Then, it uses the raw pointer. This is correct while you don’t have different threads releasing pointers. But it becomes incorrect from the moment this assumption becomes incorrect. I prefer not to make such assumptions. Even because in a near future, some dreamer might attempt to make things a little more multi-threaded. And we don’t want to make his job even harder!
Yes. You can.
The same way, you can get two unique_ptr to hold the same pointer.
The same way you can call delete() on a pointer you do not “own”.
Smart pointers can be explicitly built using a raw pointer. You can use this to create a smart pointer that owns some thing you have just allocated. If you have not just “new’d” this pointer, you should not use it to construct a smart pointer. The constructors are explicit, so you do not inadvertently store a pointer you do not want to be managed.
In particular, any one that gets a pointer and simply puts it in a smart_pointer is very very lost.
But wmayer has a lot of reason to worry about this. Because if people are using shared_ptr all around and for some reason someone needs to pass a shared_ptr to some function… and all this person has is a raw pointer… this person WILL fatally construct a shared_ptr and pass it to the function. The proper shared_ptr’s will become dangling as soon as the function returns.
I do not want to throw gasoline in the fire, so I will not mention “enable_shared_from_this”… that seems to be useful in case of FreeCAD, where passing ownership is done in obscure ways. Please, do not start a discussion on “enable_shared_from_this”…
According to the warning, you should not (need not) use std::move because when you return a local variable, it is already movable.
Sometimes compilers can be very confusing. We had some cases where clang raised a warning when returning a local object (a TopoDS_Shape) that it should be moved. A workaround was then to do TopoDS_Shape(std::move(shape));
Yes. It simply makes no sense to use expired() before a lock(). You simply lock() and that’s it.
Maybe my reply wasn’t clear enough. With “correct” I meant that you were right about the possible race condition and that expired() and lock() shouldn’t be used together.
Using an intrusive pointer is nothing like those 2 examples!
Context is everything and there are plenty of contexts where this statement is wrong. 3 libraries, off the top of my head that are designed to use this pattern. Opencascade, OpenSceneGraph and VulkanSceneGraph.
Don’t worry. I don’t think you have any left after you got done gaslighting me.
I will not say what I think of this, because I have already enough conflicts here! But you know what I want to say…
I suppose you are talking about things like this. The point here is that a local variable is not movable. You do need to cast it as movable (std::move()) if you want to move it. The local variable is movable when it is returned, because it would be immediately destructed anyway. When you say
return TopoDS_Shape(result);
the compiler doesn’t really know if result should be moved, although it probably does in this case. At the point “result” is passed to TopoDS_Shape’s constructor, it is not movable.
I don’t know what an “intrusive pointer” is… but it doesn’t sound good.
In a couple of days, I hope I can post to you another example. I am doing exactly what I tell you not to do. I will post it here as soon as I get it pushed.
But the pattern might appear because:
The library was designed following the Core Guidelines, but the code that uses it does not. So, when passing ownership of a raw pointer you own (a raw pointer you own… ), you have to first wrap it around a shared_ptr or a unique_ptr, were it actually should be from the very beginning.
The library was not designed to work with shared_ptr but was “patched” to do so. Just like I want to do with FreeCAD.
The pattern does not actually appear… you thought it does.
My design will use this same pattern, because I will not refactor to the point of having the whole of FreeCAD following the Core Guidelines.
Putting a pointer you do not own in a temporary smart pointer is just like deleting it. It is a deferred delete. Of course, no one would directly delete it because everybody knows it would be wrong. What would worry me is that if some function demands a shared_ptr as argument… maybe some programmer would happily wrap its pointer in a shared_ptr so the function will accept the argument. This might delete the pointer as soon as the function returns. If this happens, it is a symptom of design problems!
Why would a holder of a raw pointer call a function that demands passing shared ownership???
I was afraid of wmayer or any other paying attention to this “enable_shared_from_this”. So… shhhhh…
I don’t know what an “intrusive pointer” is… but it doesn’t sound good.
https://www.boost.org/doc/libs/1_46_0/libs/smart_ptr/intrusive_ptr.html
The handled class itself provides a reference counter. So, the intrusive class only increments and decrements that counter and this way it’s perfectly possible that several independent intrusive pointers share the same object.
Windows passed! I really did not expect that.
I mean… except for Ubuntu 22-4. It seems to crash… probably a double-delete caused by the mixing of raw and smart pointers.
<Exception> No such attribute 'Name123'
Attribute: No such attribute 'Name123'
Program received signal SIGSEGV, Segmentation fault.
#0 /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7f7159842520]
#1 /lib/x86_64-linux-gnu/libc.so.6(+0x1b223c) [0x7f71599b223c]
#2 0x5613873e9715 in std::char_traits<char>::length(char const*) from FreeCADCmd+0x15
#3 0x5613873e6f14 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<std::allocator<char> >(char const*, std::allocator<char> const&) from FreeCADCmd+0x74
#4 0x7f715c184bdd in App::DocumentObjectPy::getName() const from /usr/local/lib/libFreeCADApp.so+0x14d
#5 0x7f715c17d8ad in App::DocumentObjectPy::staticCallback_getName(_object*, void*) from /usr/local/lib/libFreeCADApp.so+0xad
#6 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_GenericGetAttrWithDict+0x85) [0x7f715a1308a5]
#7 0x7f715b3a7e2d in Base::PyObjectBase::_getattr(char const*) from /usr/local/lib/libFreeCADBase.so+0x11d
#8 0x7f715b2910c5 in Base::BaseClassPy::_getattr(char const*) from /usr/local/lib/libFreeCADBase.so+0x2b5
#9 0x7f715b394f35 in Base::PersistencePy::_getattr(char const*) from /usr/local/lib/libFreeCADBase.so+0x2b5
#10 0x7f715c2ee6a5 in App::PropertyContainerPy::_getattr(char const*) from /usr/local/lib/libFreeCADApp.so+0x2b5
#11 0x7f715c11a635 in App::ExtensionContainerPy::_getattr(char const*) from /usr/local/lib/libFreeCADApp.so+0x2b5
#12 0x7f715c186545 in App::DocumentObjectPy::_getattr(char const*) from /usr/local/lib/libFreeCADApp.so+0x2b5
#13 0x7f715b3a72c3 in Base::PyObjectBase::__getattro(_object*, _object*) from /usr/local/lib/libFreeCADBase.so+0x173
#14 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(PyObject_GetAttr+0x3b) [0x7f715a12ff2b]
#15 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(PyObject_GetAttrString+0x43) [0x7f715a1300c3]
#16 0x7f715c16cdd0 in Py::Object::getAttr(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const from /usr/local/lib/libFreeCADApp.so+0x40
#17 0x7f715c23a5f9 in App::FeatureTestAttribute::~FeatureTestAttribute() from /usr/local/lib/libFreeCADApp.so+0x89
#18 0x7f715c23a819 in App::FeatureTestAttribute::~FeatureTestAttribute() from /usr/local/lib/libFreeCADApp.so+0x19
#19 0x7f715c0e1aac in std::_Sp_counted_ptr<App::DocumentObject*, (__gnu_cxx::_Lock_policy)2>::_M_dispose() from /usr/local/lib/libFreeCADApp.so+0x2c
#20 0x7f715c0578ca in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use() from /usr/local/lib/libFreeCADApp.so+0x1a
#21 0x7f715c0578a5 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use_cold() from /usr/local/lib/libFreeCADApp.so+0x15
#22 0x7f715c057880 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() from /usr/local/lib/libFreeCADApp.so+0x120
#23 0x7f715c05775a in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() from /usr/local/lib/libFreeCADApp.so+0x2a
#24 0x7f715c057a89 in std::__shared_ptr<App::DocumentObject, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() from /usr/local/lib/libFreeCADApp.so+0x19
#25 0x7f715bfdb8f5 in std::shared_ptr<App::DocumentObject>::~shared_ptr() from /usr/local/lib/libFreeCADApp.so+0x15
#26 0x7f715bfe0559 in std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >::~pair() from /usr/local/lib/libFreeCADApp.so+0x19
#27 0x7f715bfe0539 in void std::__new_allocator<std::__detail::_Hash_node<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, false> >::destroy<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> > >(std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >*) from /usr/local/lib/libFreeCADApp.so+0x19
#28 0x7f715bfe047d in void std::allocator_traits<std::allocator<std::__detail::_Hash_node<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, false> > >::destroy<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> > >(std::allocator<std::__detail::_Hash_node<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, false> >&, std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >*) from /usr/local/lib/libFreeCADApp.so+0x1d
#29 0x7f715bfe044a in std::__detail::_Hashtable_alloc<std::allocator<std::__detail::_Hash_node<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, false> > >::_M_deallocate_node(std::__detail::_Hash_node<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, false>*) from /usr/local/lib/libFreeCADApp.so+0x3a
#30 0x7f715bfe03c5 in std::__detail::_Hashtable_alloc<std::allocator<std::__detail::_Hash_node<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, false> > >::_M_deallocate_nodes(std::__detail::_Hash_node<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, false>*) from /usr/local/lib/libFreeCADApp.so+0x45
#31 0x7f715bfe02e6 in std::_Hashtable<App::DocumentObject*, std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, std::allocator<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> > >, std::__detail::_Select1st, std::equal_to<App::DocumentObject*>, std::hash<App::DocumentObject*>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::clear() from /usr/local/lib/libFreeCADApp.so+0x36
#32 0x7f715bfe0269 in std::_Hashtable<App::DocumentObject*, std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> >, std::allocator<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> > >, std::__detail::_Select1st, std::equal_to<App::DocumentObject*>, std::hash<App::DocumentObject*>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::~_Hashtable() from /usr/local/lib/libFreeCADApp.so+0x19
#33 0x7f715bfd40a5 in std::unordered_map<App::DocumentObject*, std::shared_ptr<App::DocumentObject>, std::hash<App::DocumentObject*>, std::equal_to<App::DocumentObject*>, std::allocator<std::pair<App::DocumentObject* const, std::shared_ptr<App::DocumentObject> > > >::~unordered_map() from /usr/local/lib/libFreeCADApp.so+0x15
#34 0x7f715bfd644d in App::DocumentP::~DocumentP() from /usr/local/lib/libFreeCADApp.so+0x8d
#35 0x7f715bfb9e7b in App::Document::~Document() from /usr/local/lib/libFreeCADApp.so+0x22b
#36 0x7f715bfba609 in App::Document::~Document() from /usr/local/lib/libFreeCADApp.so+0x19
#37 0x7f715c3e6c0c in std::default_delete<App::Document>::operator()(App::Document*) const from /usr/local/lib/libFreeCADApp.so+0x2c
#38 0x7f715c3cebdd in std::unique_ptr<App::Document, std::default_delete<App::Document> >::~unique_ptr() from /usr/local/lib/libFreeCADApp.so+0x4d
#39 0x7f715c3b4d3f in App::Application::closeDocument(char const*) from /usr/local/lib/libFreeCADApp.so+0x2af
#40 0x7f715c4025d8 in App::Application::sCloseDocument(_object*, _object*) from /usr/local/lib/libFreeCADApp.so+0xd8
#41 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x128008) [0x7f715a128008]
#42 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_MakeTpCall+0x8c) [0x7f715a0dfadc]
#43 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x9dbc) [0x7f715a07ba1c]
#44 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#45 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x829e) [0x7f715a079efe]
#46 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#47 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x829e) [0x7f715a079efe]
#48 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#49 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0xe2358) [0x7f715a0e2358]
#50 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x4b16) [0x7f715a076776]
#51 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#52 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_FastCallDictTstate+0x57) [0x7f715a0e1bf7]
#53 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_Call_Prepend+0xe0) [0x7f715a0e1e10]
#54 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x14eaa9) [0x7f715a14eaa9]
#55 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_MakeTpCall+0x8c) [0x7f715a0dfadc]
#56 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0xa62a) [0x7f715a07c28a]
#57 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#58 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0xe2358) [0x7f715a0e2358]
#59 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x4b16) [0x7f715a076776]
#60 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#61 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_FastCallDictTstate+0x57) [0x7f715a0e1bf7]
#62 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_Call_Prepend+0xe0) [0x7f715a0e1e10]
#63 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x14eaa9) [0x7f715a14eaa9]
#64 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_MakeTpCall+0x8c) [0x7f715a0dfadc]
#65 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0xa62a) [0x7f715a07c28a]
#66 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#67 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0xe2358) [0x7f715a0e2358]
#68 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x4b16) [0x7f715a076776]
#69 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#70 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_FastCallDictTstate+0x57) [0x7f715a0e1bf7]
#71 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_Call_Prepend+0xe0) [0x7f715a0e1e10]
#72 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x14eaa9) [0x7f715a14eaa9]
#73 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_MakeTpCall+0x8c) [0x7f715a0dfadc]
#74 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0xa62a) [0x7f715a07c28a]
#75 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#76 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0xe2358) [0x7f715a0e2358]
#77 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x4b16) [0x7f715a076776]
#78 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#79 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_FastCallDictTstate+0x57) [0x7f715a0e1bf7]
#80 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_Call_Prepend+0xe0) [0x7f715a0e1e10]
#81 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x14eaa9) [0x7f715a14eaa9]
#82 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyObject_MakeTpCall+0x8c) [0x7f715a0dfadc]
#83 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0xa62a) [0x7f715a07c28a]
#84 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#85 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x829e) [0x7f715a079efe]
#86 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#87 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(_PyEval_EvalFrameDefault+0x9d68) [0x7f715a07b9c8]
#88 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x1c23af) [0x7f715a1c23af]
#89 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(PyEval_EvalCode+0xbe) [0x7f715a1bd3de]
#90 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(+0x20c4cd) [0x7f715a20c4cd]
#91 /lib/x86_64-linux-gnu/libpython3.10.so.1.0(PyRun_StringFlags+0x79) [0x7f715a20d4f9]
#92 0x7f715b2fea5c in Base::InterpreterSingleton::runString[abi:cxx11](char const*) from /usr/local/lib/libFreeCADBase.so+0x16c
#93 0x7f715c3cbfae in App::Application::runApplication() from /usr/local/lib/libFreeCADApp.so+0x22e
#94 FreeCADCmd(+0x2644b) [0x5613873e644b]
#95 /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f7159829d90]
#96 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7f7159829e40]
#97 FreeCADCmd(+0x25775) [0x5613873e5775]
Error: Process completed with exit code 1.
Run sed -ne "/^\(FAILED\|ERROR\):/,/^[[:blank:]]*$/bF; /^Traceback/,/^[^[:blank:]]/{/^Traceback/bT; /^[^[:blank:]]/G; bT}; b; :T w /tmp/logs/TestCLIInstall.log_tracebacks" -e "b; :F w /tmp/logs/TestCLIInstall.log_failedtests" /tmp/logs/TestCLIInstall.log
I have spent some hours to find out that the system update I made upgraded OCCT from 7.6 to 7.7, and this was causing one test to fail.
I guess I have to write some documentation, because this mixing of ownership with raw pointers and with std::shared_ptr needs some explaining.
Right now, I am using std::enable_shared_from_this, because the way that a TransactionalObject can arrive from App::Document or Gui::Document through a Transaction can be very complicated. It was not feasible to work it all out with std::shared_ptr. In the future, I hope to get rid of “enable_shared_from_this”, so that wmayer can implement his own policy for handling weak_ptr and shared_ptr. The policy could even be “enable_shared_from_this”. So, I tried to avoid using “enable_shared_from_this” where it was possible to do so.
Some methods, like “_assumeOwnership” have a shared_ptr version and a raw pointer version. I hope the raw pointer version gets eliminated soon. I think abdullah might be interested.
When we use “enable_shared_from_this”, we want a class that is able to handle a “shared_ptr” that points to itself. However, this class shall not produce a shared_ptr to itself. Suppose you do this:
struct Bad
{
std::shared_ptr<Bad> getptr()
{
return std::shared_ptr<Bad>(this);
}
~Bad() { std::cout << "Bad::~Bad() called\n"; }
};
int main()
{
Bad* b = new Bad;
auto p1 = b->getptr(); // We are holding one shared_ptr to b.
// This will return a shared_ptr to b that has a different reference counter.
b->getptr(); // And it will make b be deleted.
// When p1 is destructed, b will be deleted again!!!
}
This is wrong, because every time “getptr()” is called, a brand new shared_ptr is produced, with is own reference counter (control block). For a shared_ptr to “share” a pointer, it needs to be passed another shared_ptr instance that will “share” the pointer and the reference counter.
Calling “std::shared_ptr my_smart{pointer}” is correct when all those are true:
The pointer was allocated (using new, for example).
The pointer is not managed by any smart pointer, yet.
You want the pointed object to be destructed when the last shared_ptr that has the same control block gets destructed.
This all is just “shared_ptr”, and it has nothing to do with “shared_from_this”.
When you want a class that is able to handle a shared_ptr to itself, you should not implement the Bad class above. To handle the shared_ptr to itself, the class needs:
To have an internal weak_ptr to itself.
To get the weak_ptr correctly initialized by the shared_ptr constructor.
That is what “enable_shared_from_this” does. The constructor of shared_ptr is designed to detect the “enable_shared_from_this” and correctly initialize its internal weak_ptr.
But…
If the Good class you mentioned is used like this…
Good* bad_good_pointer = new Good;
// This will throw std::bad_weak_ptr.
auto shared = bad_good_pointer->shared_from_this();
An exception will be thrown. The enable_shared_from_this does not produce its own shared_ptr. It is not possible. Someone else needs to be holding a valid shared_ptr.
To prevent this, the cppreference mentions a Best (better? ) class. There, the constructor is made private and a factory is implemented to never allow “new Best”. The factory will always produce a shared_ptr, and in this case “shared_from_this()” will never throw!
What about FreeCAD?
The pointers are usually new’d. When we get the pointer, it can be or not managed by some shared_ptr. I just hope it is not managed by a unique_ptr, for example… but it should not, if you are passing ownership by raw pointers!
In FreeCAD, those pointers are usually created using “new”, and the ownership is passed as a raw pointer. In this case, and only in this case, we can create our own shared_ptr using this raw pointer.
Only after we do this, shared_from_this() will return a valid shared_ptr instead of throwing.
In some places, where we want to “take ownership” but we don’t know if there is a shared_ptr already, we first try shared_from_this()… if it throws, we assume no one has a smart pointer to it and (in a racy way) we create one by passing the pointer to the constructor of std::shared_ptr. When FreeCAD start using something like the Best class and stop using new’d raw pointers… then we can get rid of this part of the code you mention.
), you have to first wrap it around a shared_ptr or a unique_ptr, were it actually should be from the very beginning.