Broken ocl::Path(const Path &) constructor

[petterreinholdtsen]

The copy constructor for ocl::Path is a no-op, not copying anything from the source object. Why is it there? As far as I can tell, the code build just fine without the copy constructor. If it is needed and wanted, what about making it a proper copy constructor which make a copy of the content of the source object.

Here is a draft patch removing the copy constructor, and including a simple implementation of the list copying.

--- opencamlib-salsa.orig/src/geo/path.cpp	2023-09-11 12:07:29.133078761 +0200
+++ opencamlib-salsa/src/geo/path.cpp	2023-09-11 12:07:29.125078733 +0200
@@ -28,9 +28,12 @@
 Path::Path() {
 }
 
+  /*
 Path::Path(const Path &p) {
+  std::copy(p.span_list.begin(), p.span_list.end(),
+	    std::back_inserter(span_list));
 }
-
+  */
 Path::~Path() {
 }
 
--- opencamlib-salsa.orig/src/geo/path.hpp	2023-09-11 12:07:29.133078761 +0200
+++ opencamlib-salsa/src/geo/path.hpp	2023-09-11 12:07:29.125078733 +0200
@@ -98,7 +98,7 @@
         /// create empty path
         Path();
         /// copy constructor
-        Path(const Path &p);
+        //Path(const Path &p);
         /// destructor
         virtual ~Path();
 

[vespakoen]

It seems to be introduced (13 years ago) over here: 5cbe842

And wasn't used, even in the first commit.
Probably a leftover that got committed by accident.

I think we should probably remove it and enable a compiler flag to show unused variables.

[vespakoen]

It does actually seems to be exported to the Python bindings, and other classes seem to have it as well, so perhaps there is / was a reason for it, I am guessing it might have to do with Pybind Boost.Python:

5cbe842#diff-0616b7317a8086d1a85467f49ed29c5fdbfde09ee5f98ead04bd224d992f8db8R143

[vespakoen]

Never mind, I wasn't aware of C++'s copy constructors, learned something new... so I guess the code makes it more clear / explicit that an shallow copy is "enough"?

What is different in your example compared to the shallow copy?

[petterreinholdtsen]

[Koen Schmeets]

Never mind, I wasn't aware of C++'s copy constructors, learned something new... so I guess the code makes it more clear / explicit that an shallow copy is "enough"?

My proposal make a shallow copy, that is correct. I am not sure if it is enough, nor correct. Perhaps a deep copy is needed? I do not really know the code enough to tell. Or perhaps copying Path should be prohibited?

What is different in your example compared to the shallow copy?

I am not quite sure, but suspect it will ensure there are slightly different span_list contents.

…

-- Happy hacking Petter Reinholdtsen