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

Issue 5015051: code review 5015051: go/build: change //build to // +build (Closed)

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

Description

go/build: change //build to // +build New rules as discussed on CL 5011046. Also apply to C and assembly files, not just Go files.

Patch Set 1 #

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

Total comments: 2

Patch Set 3 : diff -r 222eeeaa7a3c https://go.googlecode.com/hg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -25 lines) Patch
M src/pkg/go/build/dir.go View 1 2 3 chunks +69 lines, -25 lines 0 comments Download

Messages

Total messages: 9
rsc
Hello r (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg
9 years, 2 months ago (2011-09-15 18:56:43 UTC) #1
r
LGTM http://codereview.appspot.com/5015051/diff/2001/src/pkg/go/build/dir.go File src/pkg/go/build/dir.go (right): http://codereview.appspot.com/5015051/diff/2001/src/pkg/go/build/dir.go#newcode339 src/pkg/go/build/dir.go:339: f := strings.Fields(string(bytes.TrimSpace(line[len(slashslash):]))) why not use bytes.Fields?
9 years, 2 months ago (2011-09-15 19:27:36 UTC) #2
rog
http://codereview.appspot.com/5015051/diff/2001/src/pkg/go/build/dir.go File src/pkg/go/build/dir.go (right): http://codereview.appspot.com/5015051/diff/2001/src/pkg/go/build/dir.go#newcode339 src/pkg/go/build/dir.go:339: f := strings.Fields(string(bytes.TrimSpace(line[len(slashslash):]))) On 2011/09/15 19:27:37, r wrote: > ...
9 years, 2 months ago (2011-09-15 19:39:52 UTC) #3
r
On 2011/09/15 19:39:52, rog wrote: > http://codereview.appspot.com/5015051/diff/2001/src/pkg/go/build/dir.go > File src/pkg/go/build/dir.go (right): > > http://codereview.appspot.com/5015051/diff/2001/src/pkg/go/build/dir.go#newcode339 > ...
9 years, 2 months ago (2011-09-15 19:41:44 UTC) #4
rog
On 2011/09/15 19:41:44, r wrote: > On 2011/09/15 19:39:52, rog wrote: > > http://codereview.appspot.com/5015051/diff/2001/src/pkg/go/build/dir.go > ...
9 years, 2 months ago (2011-09-15 19:44:17 UTC) #5
r
i suppose anyway i did LGTM it
9 years, 2 months ago (2011-09-15 19:46:34 UTC) #6
rsc
On Thu, Sep 15, 2011 at 15:27, <r@golang.org> wrote: > why not use bytes.Fields? eventually ...
9 years, 2 months ago (2011-09-15 20:32:05 UTC) #7
rsc
*** Submitted as http://code.google.com/p/go/source/detail?r=a12d99eb48bb *** go/build: change //build to // +build New rules as discussed ...
9 years, 2 months ago (2011-09-15 20:48:26 UTC) #8
rog
9 years, 2 months ago (2011-09-15 20:56:47 UTC) #9
definitely nicer. do i sense an idiom emerging? the dual between
bytes and strings is quite nice for this kind of transformation.

On 15 September 2011 21:32, Russ Cox <rsc@golang.org> wrote:
> On Thu, Sep 15, 2011 at 15:27,  <r@golang.org> wrote:
>> why not use bytes.Fields?
>
> eventually i needed strings to call some other
> functions (like ctxt.matchOSArch).  i just delayed
> the conversion until i was sure i would need it.
> i delayed it a little more and now it looks almost normal.
>
>                if bytes.HasPrefix(line, slashslash) {
>                        line = bytes.TrimSpace(line[len(slashslash):])
>                        if len(line) > 0 && line[0] == '+' {
>                                // Looks like a comment +line.
>                                f := strings.Fields(string(line))
>
Sign in to reply to this message.

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