Skip to content

Primitive copy on maps #19

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

Open
enderv wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Primitive copy on maps #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1eb73be
add an assignment for map so primitives copy
enderv Jan 19, 2022
2e73570
test and lint fixes
enderv Jan 19, 2022
1f5685a
comment cleanup
enderv Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,10 @@ func walkType(source, sink, x string, m types.Type, w io.Writer, imports map[str
fmt.Fprintf(w, ` for %s := range %s {
`, idx, source)

b.WriteTo(w)
_, err := b.WriteTo(w)
if err != nil {
fmt.Println(err)
}

fmt.Fprintf(w, "}\n")
}
Expand Down Expand Up @@ -456,7 +459,11 @@ func walkType(source, sink, x string, m types.Type, w io.Writer, imports map[str
if b.Len() > 0 {
ksink = copyKSink
fmt.Fprintf(w, "var %s %s\n", ksink, kkind)
b.WriteTo(w)
fmt.Fprintf(w, "%s = %s\n", ksink, source)
Copy link
Copy Markdown
Collaborator

@urandom urandom Jan 20, 2022

Choose a reason for hiding this comment

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

Hi. could you tell me what the idea behind this line is? It looks like it would copy the pointer map to a new variable, but if that's the case I don't think we should be doing that, since we want copy the individual keys and values in the map itself.

Copy link
Copy Markdown
Author

@enderv enderv Jan 21, 2022

Choose a reason for hiding this comment

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

thanks for the quick reply! Sorry for lack of initial explanation: What I found was on nested custom structs it would handle maps/slices ok but not any primitive types when it got to the bottom of the map. If I run deep-copy code for each individual type it would work ok but as we have a very complex type I did not want other devs to have to always remember to generate for each new one that (they might not even know its embedded).

for k6, v6 := range o.Meta.Data.CustomTypes {
    var cp_Meta_Data_CustomTypes_v6 CustomType
    if v6.MapField != nil {
        cp_Meta_Data_CustomTypes_v6.ArrayField = make(map[string]MapFieldType, len(v6.MapField))
       ...
    }
    if v6.MapField2 != nil {
        ....
    }
    ...continues on for all other complex types but misses strings and such.

With Change:
for k6, v6 := range o.Meta.Data.CustomTypes {
    var cp_Meta_Data_CustomTypes_v6 CustomType
    cp_Meta_Data_CustomTypes_v6 = v6 // will copy over everything first and then go on to complex types
    if v6.MapField != nil {
        cp_Meta_Data_CustomTypes_v6.ArrayField = make(map[string]MapFieldType, len(v6.MapField))
       ...
    }
    if v6.MapField2 != nil {
        ....
    }

Now that you called it out though I don't think this is working as expected either as if any pointer types it would not work correctly (just don't have any in our struct so missed it) will circle back

Copy link
Copy Markdown
Collaborator

@urandom urandom Jan 21, 2022

Choose a reason for hiding this comment

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

It looks like we'll need to somehow keep and satisfy the change in main_test.go:295, but discard the rest of the test changes and make sure that no other test cases are affected

_, err := b.WriteTo(w)
if err != nil {
fmt.Println(err)
}
}
}

Expand All @@ -469,7 +476,11 @@ func walkType(source, sink, x string, m types.Type, w io.Writer, imports map[str
if b.Len() > 0 {
vsink = copyVSink
fmt.Fprintf(w, "var %s %s\n", vsink, vkind)
b.WriteTo(w)
fmt.Fprintf(w, "%s = %s\n", vsink, val)
Copy link
Copy Markdown
Collaborator

@urandom urandom Jan 20, 2022

Choose a reason for hiding this comment

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

Same question as the above

_, err := b.WriteTo(w)
if err != nil {
fmt.Println(err)
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ func (o Foo) DeepCopy() Foo {
cp.Map = make(map[string]*Bar, len(o.Map))
for k2, v2 := range o.Map {
var cp_Map_v2 *Bar
cp_Map_v2 = v2
if v2 != nil {
cp_Map_v2 = new(Bar)
*cp_Map_v2 = *v2
Expand Down Expand Up @@ -101,6 +102,7 @@ func (o *Foo) DeepCopy() *Foo {
cp.Map = make(map[string]*Bar, len(o.Map))
for k2, v2 := range o.Map {
var cp_Map_v2 *Bar
cp_Map_v2 = v2
if v2 != nil {
cp_Map_v2 = new(Bar)
*cp_Map_v2 = *v2
Expand Down Expand Up @@ -132,6 +134,7 @@ func (o *Foo) DeepCopy() *Foo {
cp.Map = make(map[string]*Bar, len(o.Map))
for k2, v2 := range o.Map {
var cp_Map_v2 *Bar
cp_Map_v2 = v2
if v2 != nil {
cp_Map_v2 = new(Bar)
*cp_Map_v2 = *v2
Expand Down Expand Up @@ -289,10 +292,12 @@ func (o SomeStruct2) DeepCopy() SomeStruct2 {
cp.mapStruct = make(map[string]SomeStruct, len(o.mapStruct))
for k2, v2 := range o.mapStruct {
var cp_mapStruct_v2 SomeStruct
cp_mapStruct_v2 = v2
if v2.mapSlice != nil {
cp_mapStruct_v2.mapSlice = make(map[string][]string, len(v2.mapSlice))
for k4, v4 := range v2.mapSlice {
var cp_mapStruct_v2_mapSlice_v4 []string
cp_mapStruct_v2_mapSlice_v4 = v4
if v4 != nil {
cp_mapStruct_v2_mapSlice_v4 = make([]string, len(v4))
copy(cp_mapStruct_v2_mapSlice_v4, v4)
Expand All @@ -317,6 +322,7 @@ func (o SomeStruct) DeepCopy() SomeStruct {
cp.mapSlice = make(map[string][]string, len(o.mapSlice))
for k2, v2 := range o.mapSlice {
var cp_mapSlice_v2 []string
cp_mapSlice_v2 = v2
if v2 != nil {
cp_mapSlice_v2 = make([]string, len(v2))
copy(cp_mapSlice_v2, v2)
Expand All @@ -334,6 +340,7 @@ func (o SomeStruct2) DeepCopy() SomeStruct2 {
cp.mapStruct = make(map[string]SomeStruct, len(o.mapStruct))
for k2, v2 := range o.mapStruct {
var cp_mapStruct_v2 SomeStruct
cp_mapStruct_v2 = v2
cp_mapStruct_v2 = v2.DeepCopy()
cp.mapStruct[k2] = cp_mapStruct_v2
}
Expand Down Expand Up @@ -411,6 +418,7 @@ func (o I12StructWithMapOfSlices) DeepCopy() I12StructWithMapOfSlices {
cp.Sc1 = make(map[string][]I12StructWithSlices, len(o.Sc1))
for k2, v2 := range o.Sc1 {
var cp_Sc1_v2 []I12StructWithSlices
cp_Sc1_v2 = v2
if v2 != nil {
cp_Sc1_v2 = make([]I12StructWithSlices, len(v2))
copy(cp_Sc1_v2, v2)
Expand Down