Make Property properly use explicit copy #3077
Conversation
It should have a private copy constructor, and vector<Property> can be copied using copyVector
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3077 +/- ##
=======================================
Coverage 93.03% 93.04%
=======================================
Files 1155 1155
Lines 43267 43268 +1
=======================================
+ Hits 40254 40257 +3
+ Misses 3013 3011 -2 ☔ View full report in Codecov by Sentry. |
| @@ -41,6 +38,11 @@ class Property { | |||
| static void toCypher( | |||
| const std::vector<kuzu::catalog::Property>& properties, std::stringstream& ss); | |||
|
|
|||
| private: | |||
| Property(const Property& other) | |||
There was a problem hiding this comment.
I understand this change.
But why was a public constructor causing a problem on Windows. I thought it will always allow copy assignment everywhere.
There was a problem hiding this comment.
It wasn't. The issue on windows was that it was generating a default operator= for TableCatalogEntry, which stores a vector<Property> and the default uses the deleted Property::operator= (maybe it always generates the default with dllexport/KUZU_API).
But I also saw this and thought it should be corrected to match the pattern, particularly since it's a little odd to have a deleted operator= and a public copy constructor.
Though now in #3071 I guess TableCatalogEntry will have the same issue, but that has a separate copy function that produces a unique_ptr which would conflict with the one produced by the EXPLICIT_COPY_METHOD macro. I suppose it might be able to also define operator= manually using copyVector and the move constructor instead of operator= for assigning the properties, but that's more work and it might be worth considering replacing the explicit copy producing a unique_ptr with the explicit copy from copy_constructors.h to be more consistent.
|
Changes from this were incorporated into #3071 |
It should have a private copy constructor, and
vector<Property>can be copied usingcopyVector