App: fix undo/redo of dynamic property add/remove/change #3625
Conversation
There was a problem hiding this comment.
This is a straightforward change. It doesn't appear to break anything, the code is compact and understandable, and based on @realthunder's description it sounds like it fixes a potentially serious problem. There is no test case given to demonstrate the issue, but changing to a monotonically increasing thread-safe integer key for this map seems like a good idea.
src/App/Property.cpp
Outdated
| @@ -48,11 +50,12 @@ TYPESYSTEM_SOURCE_ABSTRACT(App::Property , Base::Persistence) | |||
| //************************************************************************** | |||
| // Construction/Destruction | |||
|
|
|||
| static std::atomic<long> _PropID; | |||
There was a problem hiding this comment.
Could we define this to be an uint64_t instead?
On some configurations long can be 32bits.
If someone would use such a system and this static variable would get updated 60 times a second it would only take about a year before it wraps around and becomes negative. Give it another year and the uniqueness aren't guaranteed anymore.
I do know that this is an edge case, but better safe than sorry when it comes to uniqueness. Edge cases has happened before :)
berndhahnebach
added
Core
|
Following a link to the branch on the CI-repository: https://gitlab.com/freecad/FreeCAD-CI/-/commits/PR_3625 The CI-status is available on the latest commit of the branch. https://gitlab.com/freecad/FreeCAD-CI/-/pipelines?scope=branches |
|
Yes, my current self agrees with my past self (for once!) 😃 I think this is a good idea. |
|
@realthunder , please rebase this to be merged |
realthunder
force-pushed
the
FixTransactionDynamicProp
branch
from
b3ff47a
to
4a671ed
Compare
|
Thanks for rebasing |
In an transaction where dynamic property is removed, it is copied and stored. However, if there is new dynamic property added in the same transaction, it is possible for the memory allocator to reuse the just deleted memory block. This will cause hard to debug problem, because Transaction uses a map with the property pointer as key. This commit changed the key to an integer ID.