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: https://github.com/aewallin/opencamlib/commit/5cbe842ee555f4a032803ffd99e4c0e461627494

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:

https://github.com/aewallin/opencamlib/commit/5cbe842ee555f4a032803ffd99e4c0e461627494#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