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

Issue 6856126: code review 6856126: cmd/gc: do not export useless private symbols. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by remyoudompheng
Modified:
9 years, 7 months ago
Reviewers:
CC:
rsc, golang-dev, aam, DMorsing, dfc, lvd2
Visibility:
Public.

Description

cmd/gc: do not export useless private symbols. Fixes issue 4252.

Patch Set 1 #

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

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

Total comments: 5

Patch Set 4 : diff -r 52c9c412f1f2 https://go.googlecode.com/hg/ #

Total comments: 6

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

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+118 lines, -35 lines) Patch
M src/cmd/gc/export.c View 1 2 3 7 chunks +28 lines, -27 lines 0 comments Download
M src/cmd/gc/lex.c View 1 2 3 4 2 chunks +24 lines, -8 lines 0 comments Download
A test/fixedbugs/issue4252.go View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download
A test/fixedbugs/issue4252.dir/a.go View 1 2 3 4 1 chunk +35 lines, -0 lines 0 comments Download
A test/fixedbugs/issue4252.dir/main.go View 1 2 3 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 17
remyoudompheng
Hello rsc@golang.org, golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
9 years, 7 months ago (2012-12-01 16:55:24 UTC) #1
remyoudompheng
The CL should reduce the include craziness in Go builds. It is difficult to measure ...
9 years, 7 months ago (2012-12-01 17:04:08 UTC) #2
aam
just a style thing, no other comments from me. https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/export.c File src/cmd/gc/export.c (right): https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/export.c#newcode161 src/cmd/gc/export.c:161: ...
9 years, 7 months ago (2012-12-01 17:29:57 UTC) #3
DMorsing
https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/lex.c File src/cmd/gc/lex.c (right): https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/lex.c#newcode2040 src/cmd/gc/lex.c:2040: s->origpkg = builtinpkg; What about the table driven symbols ...
9 years, 7 months ago (2012-12-01 18:07:00 UTC) #4
dfc
On 2012/12/01 18:07:00, DMorsing wrote: > https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/lex.c > File src/cmd/gc/lex.c (right): > > https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/lex.c#newcode2040 > ...
9 years, 7 months ago (2012-12-02 08:41:29 UTC) #5
lvd2
https://codereview.appspot.com/6856126/diff/5001/test/declbuiltin.dir/a.go File test/declbuiltin.dir/a.go (right): https://codereview.appspot.com/6856126/diff/5001/test/declbuiltin.dir/a.go#newcode14 test/declbuiltin.dir/a.go:14: func Test() { shouldnt main.go run this one?
9 years, 7 months ago (2012-12-03 22:40:29 UTC) #6
rsc
LGTM Code looks good as long as it works. This is all very subtle. Can ...
9 years, 7 months ago (2012-12-06 05:47:53 UTC) #7
remyoudompheng
Hello rsc@golang.org, golang-dev@googlegroups.com, mirtchovski@gmail.com, daniel.morsing@gmail.com, dave@cheney.net, lvd@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-12-06 07:22:51 UTC) #8
remyoudompheng
Still have to take a look at DMorsing's comment. http://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/export.c File src/cmd/gc/export.c (right): http://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/export.c#newcode161 src/cmd/gc/export.c:161: ...
9 years, 7 months ago (2012-12-06 07:23:55 UTC) #9
dfc
On an old arm5 host this change shaved 300ms and 10mb off the build time ...
9 years, 7 months ago (2012-12-07 00:37:38 UTC) #10
DMorsing
I got around to testing this CL and it's broken in its current form. https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c ...
9 years, 7 months ago (2012-12-07 10:41:28 UTC) #11
remyoudompheng
Thanks for the careful review. https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c File src/cmd/gc/lex.c (right): https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c#newcode2019 src/cmd/gc/lex.c:2019: if(etype != Txxx && ...
9 years, 7 months ago (2012-12-07 18:55:53 UTC) #12
DMorsing
I misremembered how the code was structured. On src/cmd/gc/lex.c:2031 there's a loop that pulls in ...
9 years, 7 months ago (2012-12-07 19:06:12 UTC) #13
remyoudompheng
On 2012/12/07 19:06:12, DMorsing wrote: > I misremembered how the code was structured. On src/cmd/gc/lex.c:2031 ...
9 years, 7 months ago (2012-12-07 19:14:07 UTC) #14
remyoudompheng
Hello rsc@golang.org, golang-dev@googlegroups.com, mirtchovski@gmail.com, daniel.morsing@gmail.com, dave@cheney.net, lvd@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-12-07 19:14:17 UTC) #15
DMorsing
LGTM.
9 years, 7 months ago (2012-12-07 20:25:30 UTC) #16
remyoudompheng
9 years, 7 months ago (2012-12-08 13:43:15 UTC) #17
*** Submitted as https://code.google.com/p/go/source/detail?r=6b602ab487d6 ***

cmd/gc: do not export useless private symbols.

Fixes issue 4252.

R=rsc, golang-dev, mirtchovski, daniel.morsing, dave, lvd
CC=golang-dev
https://codereview.appspot.com/6856126
Sign in to reply to this message.

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