-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[C++] Support C++ object copies #5988
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, cool feature! Did you run sh generate_code.sh
? I think there's some generated code files missing. Mention this in the C++ docs? We should have a section that documents what changes with the different C++ standard settings.
@vglavnyy
I did run generate_code.sh and the only modified file is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good.
I need a few days to clone and test this PR locally.
Rebased and added code to emit a explicitly-defaulted default constructor for types that will have a copy/move ctor/assignment operator. |
I've examined PR code. I think it would be better to use copy-swap for generated assignment operators. This pattern has strong guarantees against exceptions (commit-or-rollback). Probably testarrayofbools.clear(); // testarrayofbools is empty, it just constructed
testarrayofbools.reserve(o.testarrayofbools.size());
for (const auto &v : o.testarrayofbools) { testarrayofbools.emplace_back(v); } Maybe add a template routine for the template<class V>
void append_clone_of_vector(const V& from, V& to) {
reserve()
for(v : from) { to.emplace_back(...) }
} |
Thanks for the feedback @vglavnyy . I'll get working on that next week after the break in the US. |
This pull request is stale because it has been open 6 months with no activity. Please comment or this will be closed in 14 days. |
This would be appreciated |
@jfroy do you have time to finish this? |
Not immediately. I had to put this on hold, but I'll in fact have some free time in May. Maybe I can finish this up. |
Any news about this PR? |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I've updated the PR to use copy-and-swap. I did not switch to a helper function for vector cloning because it didn't seem worth it and raised some annoying namespace issues. |
Improved the generator a bit to use std::vector's copy constructor for non-pointer element types. This allows more efficient copies for trivially-copyable element types (where an implementation may do a Not sure why CI / Build Kotlin is unhappy. |
Thanks for, huh, encouraging me to try to finish this. |
Seems that it is the same in master Thanks for the work! |
@aardappel I don't know Kotlin, so I can't help much there. Also, what is the semantics of copying tables? Does it simply copy references? If so, it can lead to a strange situation with mutating API, when mutating a single copy, mutates every copy. |
Table objects are deep-copied recursively. In other words, they act as value types. Any other semantic, as you point out, is broken. |
@krojew isn't Kotlin failing on your Java changes though? Or am I not understanding correctly? They both run on the same JVM and are subject to the same object/casting rules, so I wouldn't think it's so different :) |
@aardappel technically yes, but I have no idea why. It's complaining about missing symbols which do exist when using Java, so it's something kotlin specific. |
I will have some time to check whats going on with Kotlin tomorrow. Edit: I took a look and the issue is that with @krojew changes its no longer possible to use Java 8 compiler to build the java source code. I just updated CI to use Java 11, while it still emits Java 8 bytecode. I believe #6764 should be enough to fix the original issue that @krojew tried to fix, so his changes are no longer necessary, but this is something we can revert afterwards Fix is in #6783 |
Like to ping this again to see if we can get it in. |
I thought it was ready to go and waiting for review, but it seems some old Visual Studio versions have problems in CI. I'll try to spend time soon to address those. |
@jfory CI might be better now that we have drop support for VS2010. Mind updating? |
Thanks for the ping. I rebased and looks like CI is now green. I was hoping to get the time to rework the patch to deal with (very) old compiler toolchains, but looks like I won't have to. |
Ok, thanks! I'll take one more review tonight and try to get it in. |
It looks good to me, though we should add some tests. |
I've added a basic copy test by extending |
Augment the C++ generator to emit a C++ copy constructor and a by-value copy assignment operator. This is enabled by default when the C++ standard is C++11 or later. These additional functions are only emitted for objects that need it, typically tables containing other tables. These new functions are declared in the object table type and are defined as inline functions after table declarations. When these new functions are declared, a user-defined explicitly-defaulted default constructor and move constructor are also emitted. The copy assignment operator uses the copy-and-swap idiom to provide strong exception safety, at the expense of keeping 2 full table copies in memory temporarily. fixes #5783
Sorry for the delay, looks good and thanks for adding the test. Anything pending or can I submit? |
Nope good to go. |
Thank you! |
Augment the C++ generator to emit C++ copy/move constructors and assignment operators. This is enabled by default when the C++ standard is C++11 or later. These additional functions are only emitted for objects that need it.
The copy ctor and assignment operator are declared in the object table type and are defined as inline functions after table declarations.
When copy/move ctors and assignment operators are declared, a user-defined explicitly-defaulted default ctor is also emitted.