code review 162061: change command-line argument to be

change command-line argument to be
unnamed, not require -n, in line with other
entries.

[rog, 2009-11-30 09:30:56]

Hello rog (cc: r, rsc),

I'd like you to review the following change.

[rog, 2009-11-30 09:37:34]

damn, got it wrong again! no i am not the reviewer
of this code, i'm the person that reviewed the code to
change it. an ambiguity that's caught me each time so far.

i think it's made worse by the fact there are two sections
"reviewer" (singular) and "Cc" (plural). and my monday morning
brain.

anyway, i can't undo it now.
and i'm not entirely sure whether i should have created a new issue number
or edited the old one somehow. sorry.

[rsc, 2009-11-30 17:30:36]

> anyway, i can't undo it now.
> and i'm not entirely sure whether i should have created a new issue
> number
> or edited the old one somehow. sorry.

no worries.

actually you can undo it: run "hg change 162061"
and re-edit the lines.  or you can edit the lines
on the web version.

[rsc, 2009-11-30 17:31:24]

This CL should include a change to timing.sh so
that the tests run again.  Please check that

./timing.sh -test
./timing.sh chameneos

both still succeed.

[rog, 2009-11-30 18:04:28]

ah yes.

i've just changed time.sh appropriately,
and although it has problems with this:

	echo -n foo '	'$1'	'
	$1
	shift
	(/home/r/plan9/bin/time $* 2>&1 >/dev/null) |  sed 's/r.*/r/'

i'm pretty sure the problems were there before.
(my /bin/sh seems not to interpret -n; perhaps it's trying
to divine my $PATH and thinks i'm trying to be SysVish, i dunno).

also, i don't have /home/r

i'm not sure how to attach a new file to the existing issue.
perhaps i have to create a new issue?

i've also chameneos slightly so it's more like rob's original
for its arg parsing. i've uploaded the new version.
i think.



2009/11/30 Russ Cox <rsc@golang.org>:
> This CL should include a change to timing.sh so
> that the tests run again.  Please check that
>
> ./timing.sh -test
> ./timing.sh chameneos
>
> both still succeed.
>

[rsc, 2009-11-30 18:06:12]

On Mon, Nov 30, 2009 at 10:04, roger peppe <rogpeppe@gmail.com> wrote:
> ah yes.
>
> i've just changed time.sh appropriately,
> and although it has problems with this:
>
>        echo -n foo '   '$1'    '
>        $1
>        shift
>        (/home/r/plan9/bin/time $* 2>&1 >/dev/null) |  sed 's/r.*/r/'

that should be gone if you run "hg sync"
you probably want to "hg revert timing.sh" first
so that the sync goes through cleanly.

> i'm pretty sure the problems were there before.
> (my /bin/sh seems not to interpret -n; perhaps it's trying
> to divine my $PATH and thinks i'm trying to be SysVish, i dunno).
>
> also, i don't have /home/r
>
> i'm not sure how to attach a new file to the existing issue.
> perhaps i have to create a new issue?

run hg change 162061 and add the file name to the list
or run hg file 162061 filename.

russ

[rsc, 2009-11-30 20:19:58]

your diff seems to be based on an older version of
chameneosredux (the old one not the one you sent in
a few days ago).  if you click "View" on the code review
page you'll see what i mean.  the way to fix this is to
move your changes out of the way, sync, and put them back.
(the move keeps mercurial from trying to merge the
diffs together):

cd $GOROOT/test/bench
cp chameneosredux.go x.go
hg revert chameneosredux.go
hg sync
cp x.go chameneosredux.go
hg upload 162061

and let us know when you've done that so we can 
look at the diffs.  Right now it's hard to see what
really changed.

Sorry for the mechanism distractions.

[rog, 2009-11-30 22:35:14]

ok, i think i've done those things.
the diff looks ok, and timing.sh is there.

BTW "hg file 162061 filename" replaced
rather than added to the files for the issue.
i should read the manual!


2009/11/30  <rsc@golang.org>:
> your diff seems to be based on an older version of
> chameneosredux (the old one not the one you sent in
> a few days ago).  if you click "View" on the code review
> page you'll see what i mean.  the way to fix this is to
> move your changes out of the way, sync, and put them back.
> (the move keeps mercurial from trying to merge the
> diffs together):
>
> cd $GOROOT/test/bench
> cp chameneosredux.go x.go
> hg revert chameneosredux.go
> hg sync
> cp x.go chameneosredux.go
> hg upload 162061
>
> and let us know when you've done that so we can
> look at the diffs.  Right now it's hard to see what
> really changed.
>
> Sorry for the mechanism distractions.
>
> http://codereview.appspot.com/162061
>

[rsc, 2009-11-30 23:02:25]

> BTW "hg file 162061 filename" replaced
> rather than added to the files for the issue.
> i should read the manual!

that's weird; it is supposed to add.

[rsc, 2009-11-30 23:04:49]

please make the one change
and re upload and i'll apply it.
sorry for the runaround.

http://codereview.appspot.com/162061/diff/3013/2014
File test/bench/chameneosredux.go (right):

http://codereview.appspot.com/162061/diff/3013/2014#newcode38
test/bench/chameneosredux.go:38: import "fmt"
please factor the imports

import (
   "fmt";
   ..
)

[rog, 2009-11-30 23:12:54]

ok, that should be done.

i always forget that round brackets are used
for declaration factoring, not curly ones!

2009/11/30  <rsc@golang.org>:
> please make the one change
> and re upload and i'll apply it.
> sorry for the runaround.
>
>
>
> http://codereview.appspot.com/162061/diff/3013/2014
> File test/bench/chameneosredux.go (right):
>
> http://codereview.appspot.com/162061/diff/3013/2014#newcode38
> test/bench/chameneosredux.go:38: import "fmt"
> please factor the imports
>
> import (
>   "fmt";
>   ..
> )
>
> http://codereview.appspot.com/162061
>

[rsc, 2009-11-30 23:16:32]

LGTM

thanks

[rsc, 2009-11-30 23:16:44]

*** Submitted as http://code.google.com/p/go/source/detail?r=b4614c68b22e ***

change command-line argument to be
unnamed, not require -n, in line with other
entries.

R=rsc
CC=r
http://codereview.appspot.com/162061

Committer: Russ Cox <rsc@golang.org>