Open oddfacade opened 4 months ago
What is the problem in the first place ? Can you provide a example of code that demonstrate it ?
I have a custom GUI class that works roughly like this:
class MyGUI : public GUI
{
void addNumEntry(const char* label, FAUSTFLOAT* zone, FAUSTFLOAT init, FAUSTFLOAT min, FAUSTFLOAT max, FAUSTFLOAT step)
{
registerZone(zone, new MyItem(this, label, init, min, max));
}
// etc. ...
};
It's used in context where it needs to be moveable. It's hard to succinctly summarize my exact usage without explaining a bunch of context about my codebase, but suffice to say even something as simple as this would cause a double free bug:
{
MyGUI gui_a;
my_dsp.buildUserInterface(&gui_a);
{
MyGUI tmp(gui_a); // create a copy
} // copy freed
} // segfault
Why not allocating a pointer with MyGUI* gui_a = new MyGUI();
and using it instead of the object ?
Yes, I did something very similar to your suggestion to work around the issue. I appreciate you trying to help me get unstuck, but to be clear, I'm not really asking for a solution for my project. I'm just trying to report this bug I encountered and offering to fix it. I would have just opened a PR, but I wanted to get clarification on how this class is supposed to work first. Presumably this isn't intended behavior, right? If you're not meant to be able to move/copy this object then it shouldn't have those constructors defined, and if you are meant to be able to move/copy it, it shouldn't segfault when you do so.
It seems like a straightforward fix. If the clist
s are actually meant to be shared between all copies of the GUI
then changing fZoneMap
to be a std::shared_ptr<std::map<FAUSTFLOAT*, clist>>
instead of a std::map<FAUSTFLOAT*, clist*>
would fix it. The map is a private member so we know there isn't any code depending on the fact that it's a map to pointers rather than a pointer to a map.
We can try that, can you test and prepare a PR then ?
Here. Either these
clist*
should be turned into smart pointers, theGUI
class should implement a copy constructor that deep copies the map, or the default copy constructor should be deleted to force move semantics (and the destructor should be adjusted accordingly). I can make a PR if you'd like. Do you have a preferred solution?