Wed, 02/15/2023 - 19:27
Forums:
BrepAlgoAPI_BuilderAlgo has a member BOPAlgo_PaveFiller* myDSFiller which is managed RAII style using new and delete. The class has a constructor and a destructor but no copy constructor and no copy assignment operator (also no move and move assignment operator). As a consequence, this class cannot be copied or moved without creating a double free when the destructor is called:
The following code segfaults when the destructor of cutter is called, because myDSFiller has already been freed when the destructor of cutter2 was called:
TopoDS_Shape shape = ...;
TopoDS_Shape tool = ...;
auto cutter = BRepAlgoAPI_Cut(shape, tool);
auto cutter2 = cutter;
Wed, 02/15/2023 - 21:32
Great findings. Are you going to contribute patches that would mark copy constructor disabled?
Wed, 02/15/2023 - 23:28
We would have to disable the move constructor, too. But wouldn't it be better to properly implement copy and move constructors instead of disabling them?
I suppose I could contribute, it just might take me a while. I have a busy schedule right now, and I have never contributed before, so I would have to familiarize myself with the process first...
Thu, 02/16/2023 - 07:37
This class, as many others in OCCT, is not designed to be copied by value. It might require extra efforts implementing such copy sematic properly with little to no benefit.
But this is just my impression - have no idea if there are use cases where it would be really useful to copy this class by value. It is always better to make a copy by reference (using
std:: shared_ptrand friends), when this is what you really need.Thu, 02/16/2023 - 09:25
I would argue the other way around: There are a million different ways to use OCCT and having a class in your API (a base class of many of your algorithms in fact) that can only ever be used where it is instantiated and neither moved or copied without apparent reason seems like an unnecessary restriction to me. In addition, writing a move constructor is very easy, and writing a copy constructor probably is too.
I noticed this in part of my own code, where I move instances of classes representing geometric modeling operations into a generic templated wrapper class. Now in my case I can workaround this restriction. I am just saying that copying/moving OCCT algos is not unthinkable.
Thu, 02/16/2023 - 09:52
It's not my API ;). I haven't written
BrepAlgoAPI_BuilderAlgo. If I did, then I would probably share my own vision behind it's current design or considered patching it based on your feedback.Thu, 02/16/2023 - 11:31
Haha, that's right. Since we were discussing design choices I kind of just assumed you were part of the development team.