Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(200)

Issue 5266054: code review 5266054: reflect: disallow Interface method on Value obtained vi... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 5 months ago by rsc
Modified:
12 years, 5 months ago
Reviewers:
r
CC:
r2, golang-dev
Visibility:
Public.

Description

reflect: disallow Interface method on Value obtained via unexported name Had been allowing it for use by fmt, but it is too hard to lock down. Fix other packages not to depend on it.

Patch Set 1 #

Patch Set 2 : diff -r 5ebff4e7b666 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 3 : diff -r 089e591ec81e https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 089e591ec81e https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 089e591ec81e https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r bdd5d1a91f43 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 7 : diff -r f03bb6e861a4 https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r 3dde805f9a13 https://go.googlecode.com/hg/ #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+288 lines, -167 lines) Patch
M src/pkg/fmt/fmt_test.go View 1 5 chunks +8 lines, -8 lines 0 comments Download
M src/pkg/fmt/print.go View 1 2 3 22 chunks +154 lines, -97 lines 1 comment Download
M src/pkg/reflect/all_test.go View 1 4 chunks +90 lines, -25 lines 0 comments Download
M src/pkg/reflect/deepequal.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/reflect/value.go View 1 3 chunks +15 lines, -16 lines 0 comments Download
M test/interface/fake.go View 1 2 chunks +20 lines, -20 lines 0 comments Download

Messages

Total messages: 18
rsc
Hello r (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
12 years, 5 months ago (2011-10-17 18:50:07 UTC) #1
r
http://codereview.appspot.com/5266054/diff/1001/src/pkg/fmt/print.go File src/pkg/fmt/print.go (right): http://codereview.appspot.com/5266054/diff/1001/src/pkg/fmt/print.go#newcode654 src/pkg/fmt/print.go:654: p.fmtBool(f, verb, field, reflect.Value{}) there's a lot of creation ...
12 years, 5 months ago (2011-10-17 18:59:48 UTC) #2
rsc
On Oct 17, 2011 at 14:59, r@golang.org wrote: > http://codereview.appspot.com/5266054/diff/1001/src/pkg/fmt/print.go#newcode654 > src/pkg/fmt/print.go:654: p.fmtBool(f, verb, field, ...
12 years, 5 months ago (2011-10-17 19:01:59 UTC) #3
rsc
Hello r@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
12 years, 5 months ago (2011-10-17 19:13:04 UTC) #4
rsc
Hello r@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
12 years, 5 months ago (2011-10-17 19:13:23 UTC) #5
rsc
Moving exp/template/html change into its own CL, so that this CL is only about reflect ...
12 years, 5 months ago (2011-10-17 19:49:23 UTC) #6
r2
can you please see how the fmt benchmarks are affected? -rob
12 years, 5 months ago (2011-10-17 19:57:02 UTC) #7
r
http://codereview.appspot.com/5266054/diff/12002/src/pkg/fmt/print.go File src/pkg/fmt/print.go (right): http://codereview.appspot.com/5266054/diff/12002/src/pkg/fmt/print.go#newcode667 src/pkg/fmt/print.go:667: p.fmtBool(f, verb, field, reflect.Value{}) this new repetition in already ...
12 years, 5 months ago (2011-10-17 19:59:49 UTC) #8
rsc
On Mon, Oct 17, 2011 at 15:59, <r@golang.org> wrote: > is it still possible to ...
12 years, 5 months ago (2011-10-17 20:00:44 UTC) #9
r2
On Oct 17, 2011, at 1:00 PM, Russ Cox wrote: > On Mon, Oct 17, ...
12 years, 5 months ago (2011-10-17 20:01:22 UTC) #10
rsc
On Mon, Oct 17, 2011 at 16:01, Rob 'Commander' Pike <r@google.com> wrote: > no, i ...
12 years, 5 months ago (2011-10-17 20:01:56 UTC) #11
r2
On Oct 17, 2011, at 1:01 PM, Russ Cox wrote: > On Mon, Oct 17, ...
12 years, 5 months ago (2011-10-17 20:07:33 UTC) #12
rsc
> i understand the intent, it's the implementation i'm trying to figure out. the only ...
12 years, 5 months ago (2011-10-17 22:29:08 UTC) #13
rsc
Benchmarked: it is about 5% slower. It might be interesting to move more of the ...
12 years, 5 months ago (2011-10-17 22:38:36 UTC) #14
r
LGTM but i'm not happy
12 years, 5 months ago (2011-10-17 22:42:57 UTC) #15
rsc
*** Submitted as http://code.google.com/p/go/source/detail?r=f798c2579fbd *** reflect: disallow Interface method on Value obtained via unexported name ...
12 years, 5 months ago (2011-10-17 22:48:48 UTC) #16
r
http://codereview.appspot.com/5266054/diff/18002/src/pkg/fmt/print.go File src/pkg/fmt/print.go (right): http://codereview.appspot.com/5266054/diff/18002/src/pkg/fmt/print.go#newcode667 src/pkg/fmt/print.go:667: p.fmtBool(f, verb, field, reflect.Value{}) i asked you to tidy ...
12 years, 5 months ago (2011-10-17 22:53:29 UTC) #17
rsc
12 years, 5 months ago (2011-10-18 12:41:14 UTC) #18
On Mon, Oct 17, 2011 at 18:53,  <r@golang.org> wrote:
> src/pkg/fmt/print.go:667: p.fmtBool(f, verb, field, reflect.Value{})
> i asked you to tidy this up.
>
> ok, i'll do a pass to restore it to my comfort zone.

sorry, i planned to do that in the refactoring i promised
in a later CL.  i was going to take a crack at that today
but i will assume it's in your court unless i hear otherwise.

as for reflect.Value{} vs declaring and using a zero,
the compiler should have an easier job in the former case,
because it can synthesize zeros more easily than copy them.
if it doesn't do that well, i should fix that.  i started on making
some of the init stuff a bit cleaner a while back but only got
as far as fixing globals.

russ
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b