-
Notifications
You must be signed in to change notification settings - Fork 923
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
Use ICloneable in generated code #2044
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2044 +/- ##
==========================================
- Coverage 58.18% 57.96% -0.22%
==========================================
Files 324 324
Lines 61743 61677 -66
==========================================
- Hits 35926 35752 -174
- Misses 25817 25925 +108
|
Marked as draft until gates open for 1.4.372 PR... |
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.
the changes seem to be alright in my opinion. One comment would be, hiding MemberwiseClone may have adverse effects as it is intended to do a shallow copy, instead of a deep copy that we intend to make here. Other than that, all look fine.
@@ -1482,7 +1482,7 @@ public void FetchNamespaceTables() | |||
ResponseHeader responseHeader = this.Read( | |||
null, | |||
0, | |||
TimestampsToReturn.Both, |
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.
Is this change intended?
Seems to have nothing to do with IClone.
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.
Thats correct. Intended yes, but not for IClone. Thanks for checking...
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.
Just to clarify:
This change was already merged to the master branch. There I have seen it.
I do not know if this is a problem, i just wanted to give notification to prevent mistakes.
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.
Its a tiny optimization, because the timestamps are not needed here.
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.
Ok, thanks.
- ICloneable was removed from generated code for the first port to .NET Standard 1.x, instead MemberwiseClone was used - Reenable ICloneable for generated code - Add ICloneable to built in types - Fix code where MemberwiseClone is called instead of Clone
Proposed changes
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.