fix: fixes for various errors related to shared_from_this.#171
Conversation
A while back there were some changes to the pattern language library that changed the way shared_pointers are created using shared_from_this(). Unfortunatelly the changes were not complete and various bugs were created among them 2234, json type not working, unable to export files, static arrays of bitfields,... The cause of the errors was that in class Pattern the member m_parent was left as a raw pointer and it needs to be handled by shared pointers. Also there were some cases in which share pointers were needed but unique pointers were used instead. Both cause crashes when shared_from_this is used on pointers that are not managed by shared_ptr. Another source of errors were infinite loops of clone and reference that caused stack overflow. The fixes include making m_parent a weak pointer, turning unique pointers into shared pointers and moving codefrom the copy constructors into clone to break the infinite loops.These changes are the bare minimum needed to bring the pattern language back to the full functionality that it had before shared_from_this was introduced or at least thats the hope.
|
The acknowledgment would go something like this: "Despite several attempts to engage the author of a PR that started out as a fix for the same problems but grew into a complete refactor of the system that handles the creation of shared pointers, he showed no interest in collaborating in the writing and testing of a simpler solution that just fix the bugs, even when told that there was tentative code based on his code with the unnecessary parts removed. As a result of the lack of interest all the code that was based in that pr was removed and a completely new version written from scratch was created as explained in a post made to the pr in question. The code presented here is the result of that personal effort" A quote from my post informing you of the steps I took to write this pr: |
Ok, lets see.
The feeling that you had that your work was being used without credit was misplaced. As a result you posted statements that implied misconduct on my part. You had no justification to allege misconduct from me. That is not a polite thing to do.If you really had good intentions, you would have first asked about the code's origins before making any allegations.
And here you are again attributing my remarks without indicating exactly how I was rude or where the sarcasm lies. Based on what I said in this post my response was appropriate and to the point. I need to defend myself against false allegations and I need to present evidence to back up my defense. This could have been avoided if you wouldn't have jumped the gun and started making false allegations right off the bat. Even after that you are escalating the conflict instead of admitting that your allegations were false and apologizing for it. There are several comments posted in your pr that are also aimed at making me reply negatively as a way to start conflict. I ignore them for what they are and will continue to do so but false allegations cross a line. I have explained all my actions to you and you have claimed to understand and accept them. that should be the end of the contingency as further elevations cannot be justified. |
|
Part of your problem is that you dont seem to read the post I made in order to understand why I say that the pr code is not based on your code. I did write code based on your and then i started from scratch again when I felt that you showed no interest in collaborating. I even quoted myself the exact sentence describing how I created the code.I will quote it again now. Please read it before saying any more false accusations that you have no right to make. "Starting from master i added code to reference() that throws an error if the weak pointer is expired. that finds all the cases when shared pointers need to be created. Starting from master means that any previous modifications were deleted and that there was no code left that belonged to your pr. You can ignore me or the things I say. you cannot state false claims based on that though because you are falsely accusing me. You are not the victim. Read all the posts and try to understand what is in them instead of assuming you know what i going on. |
|
I have found evidence of your false accusations. |
Thats the typical cop out from those guilty that are afraid to face the music. It only makes you look worse.
That was code that I used to attempt to bring you into the collaboration. when it became obvious that you woudn't I wanted to create a fix for the bugs in master so i started to work on master discarding all your code.
The amount of effort you put is irrelevant when your code is not being used. I had to take your code, remove all the parts that werent needed then delete all of that and start all over again because you didn't want to collaborate. You are not the only person consuming time.
That is a blatant lie and you have no way to verify that it is true. Your code was deleted and I started from scratch. I posted a note indicating that in your pr which you are conveniently ignoring. I don't lie if I dont have to and if what you are implying was true I have no problem admitting it but it isn't and I have proven it. You continue to accuse me and disregard my evidence which indicates an agenda beyond the issue at hand. There is evidence that you are trying to make me look bad in posts made by you in your pr so unless you stop this it is only going to work against you in the long run.
I have the right to defend myself against false accusations and you can't take that away.
I am questioning your entire approach to this. The point in arguing is to give people a chance to explain why the accusations are false. You cannot ignore them without becoming unfair. My conduct is beyond reproach and yours is not. You accuse falsely and ignore evidence presented to you against your claims. |
…and looked at the various bugs under the debugger and found two types of problems that made ImHex crash. The bug reported in the issue is created because enable_shared_from_this seems to affect the dynamic cast convertion. I was able to patch both places where the error occurs (one for data window and other for the imhex tooltip) For the other type of problems I turned reference() into a detection tool for invalid uses of shared_from_this and ran about a dozen of patterns and found that there is only one line on each constructor that causes crashes. The constructor can call shared_from_this on any object except the ones that detect and store the parent reference on each of the children. typically they are member->setParent(reference()) or something similar to that. Only about a dozen or so changes to 6 files in 1 repository are needed to fix all the bugs created by the introduction of shared_from_this in the code. Pinpointing the source of the errors allows us also to avoid moving code out of the constructors that doesn't need to be moved which in turn makes it safe to call constructors with the understanding that references to the childrens parent will not be created automatically and needs to be done after construction (most likely in clone()). We obtain the same level of safety and prevent adding bugs in the future with only a fraction of the changes which increases code stability and also reduces the risk of adding new bugs that always comes with massive changes to the code base.
…mic cast. Also json types needed to be set to shared_ptrs instead of unique.
* fix: fixes for various errors related to shared_from_this. A while back there were some changes to the pattern language library that changed the way shared_pointers are created using shared_from_this(). Unfortunatelly the changes were not complete and various bugs were created among them 2234, json type not working, unable to export files, static arrays of bitfields,... The cause of the errors was that in class Pattern the member m_parent was left as a raw pointer and it needs to be handled by shared pointers. Also there were some cases in which share pointers were needed but unique pointers were used instead. Both cause crashes when shared_from_this is used on pointers that are not managed by shared_ptr. Another source of errors were infinite loops of clone and reference that caused stack overflow. The fixes include making m_parent a weak pointer, turning unique pointers into shared pointers and moving codefrom the copy constructors into clone to break the infinite loops.These changes are the bare minimum needed to bring the pattern language back to the full functionality that it had before shared_from_this was introduced or at least thats the hope. * build: unit tests * I got tired of not knowing what was broken, so I went back to master and looked at the various bugs under the debugger and found two types of problems that made ImHex crash. The bug reported in the issue is created because enable_shared_from_this seems to affect the dynamic cast convertion. I was able to patch both places where the error occurs (one for data window and other for the imhex tooltip) For the other type of problems I turned reference() into a detection tool for invalid uses of shared_from_this and ran about a dozen of patterns and found that there is only one line on each constructor that causes crashes. The constructor can call shared_from_this on any object except the ones that detect and store the parent reference on each of the children. typically they are member->setParent(reference()) or something similar to that. Only about a dozen or so changes to 6 files in 1 repository are needed to fix all the bugs created by the introduction of shared_from_this in the code. Pinpointing the source of the errors allows us also to avoid moving code out of the constructors that doesn't need to be moved which in turn makes it safe to call constructors with the understanding that references to the childrens parent will not be created automatically and needs to be done after construction (most likely in clone()). We obtain the same level of safety and prevent adding bugs in the future with only a fraction of the changes which increases code stability and also reduces the risk of adding new bugs that always comes with massive changes to the code base. * build: unit tests * Needed to set m_parent to weak_ptr in order to fix problems with dynamic cast. Also json types needed to be set to shared_ptrs instead of unique. * build: unit tests * build: unit tests * build: unit tests * unique ptr that should have been shared. * undoing unneeded changes * build: unit tests
1 participant
A while back there were some changes to the pattern language library that changed the way shared_pointers are created using shared_from_this(). Unfortunatelly the changes were not complete and various bugs were created among them 2234, json type not working, unable to export files, static arrays of bitfields,... The cause of the errors was that in class Pattern the member m_parent was left as a raw pointer and it needs to be handled by shared pointers. Also there were some cases in which share pointers were needed but unique pointers were used instead. Both cause crashes when shared_from_this is used on pointers that are not managed by shared_ptr. Another source of errors were infinite loops of clone and reference that caused stack overflow. The fixes include making m_parent a weak pointer, turning unique pointers into shared pointers and moving codefrom the copy constructors into clone to break the infinite loops.These changes are the bare minimum needed to bring the pattern language back to the full functionality that it had before shared_from_this was introduced or at least thats the hope.