code review 5372095: allow copy of struct containing unexported fields (issue 5372095)
r...@golang.org
Message:
Hello golan...@googlegroups.com,
I'd like you to review this change to
https://go.googlecode.com/hg
Description:
allow copy of struct containing unexported fields
An experiment: allow structs to be copied even if they
contain unexported fields. This gives packages the
ability to return opaque values in their APIs, like reflect
does for reflect.Value but without the kludgy hacks reflect
resorts to.
In general, we trust programmers not to do silly things
like *x = *y on a package's struct pointers, just as we trust
programmers not to do unicode.Letter = unicode.Digit,
but packages that want a harder guarantee can introduce
an extra level of indirection, like in the changes to os.File
in this CL.
All in one CL so that it can be rolled back more easily if
we decide this is a bad idea.
Originally discussed in March 2011.
https://groups.google.com/group/golang-dev/t/3f5d30938c7c45ef
Please review this at http://codereview.appspot.com/5372095/
Affected files:
M doc/go_spec.html
M src/cmd/gc/go.h
M src/cmd/gc/subr.c
M src/cmd/gc/typecheck.c
M src/pkg/os/file_plan9.go
M src/pkg/os/file_unix.go
M src/pkg/os/file_windows.go
M test/assign.go
R test/fixedbugs/bug226.dir/x.go
R test/fixedbugs/bug226.dir/y.go
R test/fixedbugs/bug226.go
R test/fixedbugs/bug310.go
R test/fixedbugs/bug359.go
R test/fixedbugs/bug378.go
a...@golang.org
I'm pretty happy with how much this removes from the spec, compiler, and
tests. Looking forward to seeing what effect, if any, this has on client
code.
http://codereview.appspot.com/5372095/diff/2003/src/pkg/os/file_plan9.go
File src/pkg/os/file_plan9.go (right):
http://codereview.appspot.com/5372095/diff/2003/src/pkg/os/file_plan9.go#newcode13
src/pkg/os/file_plan9.go:13: type File {
Missing 'struct'
dvy...@google.com
copying. I am worry about structs like Mutexes. Copying is almost always
the wrong thing for them. Moreover what if it gets copied in a locked
state? Sharing and dynamic allocation is both expensive and does not
solve the problem. Is it just becomes a users problem?
In C++ I can prevent copying of mutexes, moreover I still can return
them by value thanks to move constructors.
Russ Cox
> Additional level of indirection enforces sharing rather than prevents
> copying. I am worry about structs like Mutexes. Copying is almost always
> the wrong thing for them. Moreover what if it gets copied in a locked
> state? Sharing and dynamic allocation is both expensive and does not
> solve the problem. Is it just becomes a users problem?
Generally, if an API is returning pointers (like *os.File)
you should continue to use those pointers instead of making
copies.
It is true that we are giving up a capability here (prevent copying)
but we are gaining a capability too (allow APIs with opaque values).
We have a growing list of cases where the latter would be useful,
and not too many examples where the former is critical.
It is probably worth noting in the sync docs that mutexes
should not be copied. I will add that to this CL.
> In C++ I can prevent copying of mutexes, moreover I still can return
> them by value thanks to move constructors.
It is true: C++ can prevent many things.
Russ
dvy...@google.com
> On Mon, Nov 14, 2011 at 01:26, <mailto:dvy...@google.com> wrote:
> > Additional level of indirection enforces sharing rather than
prevents
> > copying. I am worry about structs like Mutexes. Copying is almost
always
> > the wrong thing for them. Moreover what if it gets copied in a
locked
> > state? Sharing and dynamic allocation is both expensive and does not
> > solve the problem. Is it just becomes a users problem?
> Generally, if an API is returning pointers (like *os.File)
> you should continue to use those pointers instead of making
> copies.
> It is true that we are giving up a capability here (prevent copying)
> but we are gaining a capability too (allow APIs with opaque values).
> We have a growing list of cases where the latter would be useful,
> and not too many examples where the former is critical.
> It is probably worth noting in the sync docs that mutexes
> should not be copied. I will add that to this CL.
OK, I see.
Yes, I think we should document what sync objects are copyable and what
are not (WaitGroup, Once, etc). If an object with a non-trivial
multi-threaded semantics becomes copyable we should document how copying
interacts with muti-threading as well.
r...@golang.org
i think
http://codereview.appspot.com/5372095/diff/2018/doc/go_spec.html
File doc/go_spec.html (left):
http://codereview.appspot.com/5372095/diff/2018/doc/go_spec.html#oldcode1377
doc/go_spec.html:1377: </p>
this is good, but is there another place where it needs to be explicitly
allowed? that is, is there another sentence that implies this, with less
precision?
Russ Cox
> this is good, but is there another place where it needs to be explicitly
> allowed? that is, is there another sentence that implies this, with less
> precision?
I don't believe there is. Atom symbol pointed out back
in March that it's not implied by any of the rest of the spec,
which is what got me thinking about it.
Russ
Brad Fitzpatrick
On Mon, Nov 14, 2011 at 01:26, <dvy...@google.com> wrote:Generally, if an API is returning pointers (like *os.File)
> Additional level of indirection enforces sharing rather than prevents
> copying. I am worry about structs like Mutexes. Copying is almost always
> the wrong thing for them. Moreover what if it gets copied in a locked
> state? Sharing and dynamic allocation is both expensive and does not
> solve the problem. Is it just becomes a users problem?
you should continue to use those pointers instead of making
copies.
It is true that we are giving up a capability here (prevent copying)
but we are gaining a capability too (allow APIs with opaque values).
We have a growing list of cases where the latter would be useful,
and not too many examples where the former is critical.
It is probably worth noting in the sync docs that mutexes
should not be copied. I will add that to this CL.
Jan Mercl
In general, we trust programmers not to do silly things
like *x = *y on a package's struct pointers, just as we trust
programmers not to do unicode.Letter = unicode.Digit,
Russ Cox
> Considering how many silly questions/attempts to do incredibly wrong things
> are routinely discussed at golang-nuts - I'm a bit skeptical about such
> optimism. It will give more power to responsible programmers while
> increasing the potential to bite the others badly. I'm not able to estimate
> the ratio of those two populations.
This CL is not about giving power to responsible programmers.
It is about letting packages create a new, useful kind of API:
something returning an opaque struct value.
A side effect is that programmers can write a couple
more bad programs.
Russ
g...@golang.org
r...@golang.org
http://code.google.com/p/go/source/detail?r=6f8b9654e929 ***
allow copy of struct containing unexported fields
An experiment: allow structs to be copied even if they
contain unexported fields. This gives packages the
ability to return opaque values in their APIs, like reflect
does for reflect.Value but without the kludgy hacks reflect
resorts to.
In general, we trust programmers not to do silly things
like *x = *y on a package's struct pointers, just as we trust
programmers not to do unicode.Letter = unicode.Digit,
but packages that want a harder guarantee can introduce
an extra level of indirection, like in the changes to os.File
in this CL or by using an interface type.
All in one CL so that it can be rolled back more easily if
we decide this is a bad idea.
Originally discussed in March 2011.
https://groups.google.com/group/golang-dev/t/3f5d30938c7c45ef
R=golang-dev, adg, dvyukov, r, bradfitz, jan.mercl, gri
CC=golang-dev
http://codereview.appspot.com/5372095