Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add CopyTo method for deep copy #512

Merged
merged 5 commits into from
Sep 7, 2023
Merged

Add CopyTo method for deep copy #512

merged 5 commits into from
Sep 7, 2023

Conversation

smallhive
Copy link
Contributor

closes #194

@smallhive smallhive marked this pull request as ready for review September 5, 2023 08:31
cthulhu-rider
cthulhu-rider reviewed Sep 5, 2023
View reviewed changes
Copy link
Contributor

@cthulhu-rider cthulhu-rider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about object.Object?

object/id/id.go Outdated Show resolved Hide resolved
version/version.go Outdated Show resolved Hide resolved
netmap/policy.go Outdated Show resolved Hide resolved
netmap/policy.go Outdated Show resolved Hide resolved
netmap/policy.go Outdated Show resolved Hide resolved
eacl/table.go Outdated Show resolved Hide resolved
session/common.go Outdated Show resolved Hide resolved
session/container.go Outdated Show resolved Hide resolved
session/object.go Outdated Show resolved Hide resolved
netmap/policy_internal_test.go Show resolved Hide resolved
@smallhive smallhive force-pushed the 194-document-copying-of-type-instances branch from e01dc3c to 60101e0 Compare September 6, 2023 07:51
@smallhive
Copy link
Contributor Author

what about object.Object?

I wasn't confident about it. But added it now

@smallhive smallhive force-pushed the 194-document-copying-of-type-instances branch 2 times, most recently from 499e44c to f5d679a Compare September 6, 2023 07:59
cthulhu-rider
cthulhu-rider reviewed Sep 6, 2023
View reviewed changes
object/object.go
return
}

id := (*object.Object)(&o).GetObjectID()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id, sig and header have reference fields, why don't we deep copy them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I will update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, Now we have a monster inside SDK.

I tend to think we should implement this CopyTo inside api-go lib, because SDK in many cases uses types from it. Not for all types, but for most frequently used in SDK. For instance, take from this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create an issue for api-go, but for now we need some way to handle this in SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nspcc-dev/neofs-api-go#432

@smallhive smallhive force-pushed the 194-document-copying-of-type-instances branch 3 times, most recently from e5f5668 to 2807cdd Compare September 7, 2023 07:18
roman-khimov
roman-khimov reviewed Sep 7, 2023
View reviewed changes
object/object.go
return
}

id := (*object.Object)(&o).GetObjectID()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create an issue for api-go, but for now we need some way to handle this in SDK.

object/object_internal_test.go Show resolved Hide resolved
@smallhive smallhive force-pushed the 194-document-copying-of-type-instances branch from 2807cdd to 5bb7572 Compare September 7, 2023 09:50
@smallhive smallhive force-pushed the 194-document-copying-of-type-instances branch from 5bb7572 to 08dffcd Compare September 7, 2023 11:08
@smallhive smallhive force-pushed the 194-document-copying-of-type-instances branch from 08dffcd to 731030a Compare September 7, 2023 11:15
carpawell
carpawell reviewed Sep 7, 2023
View reviewed changes
object/object_copy.go
v2session "github.com/nspcc-dev/neofs-api-go/v2/session"
)

func copyByteSlice(sl []byte) []byte {
Copy link
Member

@carpawell carpawell Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you copy a slice of something more than once, how about 😎generic😎 copySlice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave it for 1.21.

Copy link
Member

@carpawell carpawell Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with 1.21 it is even better and (if my suggestion is applied) could be fixed with sed (the current version will require more work). also, that is just more boilerplate code
not critical for me

@carpawell
Copy link
Member

carpawell commented Sep 7, 2023 •
edited
Loading

BTW, I am not sure dropping deep copy method for the structs that do not need it for now is a must. If it had been already done I wouldn't have dropped it. Looks like a good practice to provide CopyTo to all the structs (we may change their implementations) or to none of them.

@roman-khimov roman-khimov mentioned this pull request Sep 7, 2023
Closed
@roman-khimov roman-khimov merged commit 942f42f into master Sep 7, 2023
@roman-khimov roman-khimov deleted the 194-document-copying-of-type-instances branch September 7, 2023 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@carpawell carpawell carpawell approved these changes

@cthulhu-rider cthulhu-rider Awaiting requested review from cthulhu-rider cthulhu-rider is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document copying of type instances
4 participants
@smallhive @carpawell @roman-khimov @cthulhu-rider