replace []Issue with []*Issue #180

Closed
rsc opened this Issue Apr 9, 2015 · 2 comments

Projects

None yet

3 participants

@rsc
Contributor
rsc commented Apr 9, 2015

In general having slices of large structs is awkward because it means that when you range over the slice, you either have to write:

for i := range list {
    x := &list[i]
    ...
}

or you write

for _, x := range list {
    ...
}

and put up with the fact that (1) the iteration is doing a (more expensive than pointer copy) struct copy for each element and (2) you cannot modify the elements by changing x, since x is a copy.

If list were a []*github.Issue it wouldn't have these problems.

The encoding/json package can handle slices of pointers just fine, but changing the types would have the drawback of breaking existing clients.

@gmlewis
Member
gmlewis commented Jun 17, 2016

While scanning through the old issues, I just found this one and think it is worthwhile doing since this package is frequently handling slices of large structs.

I'm thinking that the impact on existing clients will be negligible due to the nice inference properties of Go.

@willnorris and @shurcooL - I would like to do this... any objections?

@gmlewis gmlewis self-assigned this Jun 17, 2016
@gmlewis gmlewis added a commit to gmlewis/go-github that referenced this issue Jun 17, 2016
@gmlewis gmlewis replace []Issue with []*Issue and for other large structs as well
Note that this is an API-breaking change but should have minimal
impact on users of this package due to the nice inference
properties of the Go programming language.

Fixes #180.

Change-Id: Ib386135e6b8f306d1f54278968c576f3ceccc4e7
3b471d4
@gmlewis gmlewis added a commit to gmlewis/go-github that referenced this issue Jun 17, 2016
@gmlewis gmlewis replace []Issue with []*Issue and for other large structs as well
Note that this is an API-breaking change but should have minimal
impact on users of this package due to the nice inference
properties of the Go programming language.

Fixes #180.

Change-Id: Ib386135e6b8f306d1f54278968c576f3ceccc4e7
4ba4058
@shurcooL
Collaborator

SGTM. This looks like an improvement, and updating for API change is easy (in most cases, no changes need to be done).

@gmlewis gmlewis added a commit to gmlewis/go-github that referenced this issue Jun 17, 2016
@gmlewis gmlewis replace []Issue with []*Issue and for other large structs as well
Note that this is an API-breaking change but should have minimal
impact on users of this package due to the nice inference
properties of the Go programming language.

Fixes #180.

Change-Id: Ib386135e6b8f306d1f54278968c576f3ceccc4e7
0d70f6a
@gmlewis gmlewis added a commit to gmlewis/go-github that referenced this issue Jun 17, 2016
@gmlewis gmlewis replace []Issue with []*Issue and for other large structs as well
Note that this is an API-breaking change but should have minimal
impact on users of this package due to the nice inference
properties of the Go programming language.

Bumped `libraryVersion` to 0.2 due to API-breaking change as
discussed in #376.

Fixes #180.

Change-Id: Ib386135e6b8f306d1f54278968c576f3ceccc4e7
d4963eb
@gmlewis gmlewis added a commit to gmlewis/go-github that referenced this issue Jun 20, 2016
@gmlewis gmlewis replace []Issue with []*Issue and for other large structs as well
Note that this is an API-breaking change but should have minimal
impact on users of this package due to the nice inference
properties of the Go programming language.

Bumped `libraryVersion` to `2` due to API-breaking change as
discussed in #376.

Fixes #180.

Change-Id: Ib386135e6b8f306d1f54278968c576f3ceccc4e7
4ea1b10
@gmlewis gmlewis closed this in #375 Jun 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment