code review 10195044: lib9: avoid all -Wconversion warnings

lib9: avoid all -Wconversion warnings

Built after adding -Wconversion to the list of compiler
arguments used when building.  I believe these are all OK
assuming we will not change the API.  There is no effort to
detect overflow due to very long strings.

[iant, 2013-06-12 15:33:13]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[iant, 2013-06-12 15:42:21]

On Wed, Jun 12, 2013 at 8:33 AM,  <iant@golang.org> wrote:
>
> Description:
> lib9: avoid all -Wconversion warnings

By the way, I wouldn't commit this CL as is, I would go back and check
for any conversions that might be hiding any issues.  This CL is meant
as an indicator of where things would change, to see if people think
the changes would be too large.

Ian

[dave_cheney.net, 2013-06-12 16:00:33]

I am always a hearty plus one on this class of change. Thank you for tackling
this. 



On 13/06/2013, at 1:33, iant@golang.org wrote:

> Reviewers: golang-dev1,
> 
> Message:
> Hello golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/
> 
> 
> Description:
> lib9: avoid all -Wconversion warnings
> 
> As an experiment I tried adding -Wconversion to the list of
> compiler arguments used when building, and tried building the
> tree.  This CL fixes all the warnings in the lib9 directory.
> I didn't closely examine them to see if any of the warnings
> were real, I mostly just added type casts.
> 
> The idea here is that if we fix all these warnings, we can
> enable -Wconversion always, and perhaps catch real problems.
> 
> Any opinions as to whether this is worth doing?
> 
> Please review this at https://codereview.appspot.com/10195044/
> 
> Affected files:
>  M src/lib9/_p9dir.c
>  M src/lib9/atoi.c
>  M src/lib9/await.c
>  M src/lib9/dirfstat.c
>  M src/lib9/dirfwstat.c
>  M src/lib9/dirstat.c
>  M src/lib9/dirwstat.c
>  M src/lib9/execl.c
>  M src/lib9/flag.c
>  M src/lib9/fmt/dofmt.c
>  M src/lib9/fmt/dorfmt.c
>  M src/lib9/fmt/fltfmt.c
>  M src/lib9/fmt/fmt.c
>  M src/lib9/fmt/fmtdef.h
>  M src/lib9/fmt/fmtfdflush.c
>  M src/lib9/fmt/fmtquote.c
>  M src/lib9/fmt/fmtrune.c
>  M src/lib9/fmt/sprint.c
>  M src/lib9/fmt/strtod.c
>  M src/lib9/fmt/vsmprint.c
>  M src/lib9/fmt/vsnprint.c
>  M src/lib9/getwd.c
>  M src/lib9/readn.c
>  M src/lib9/rfork.c
>  M src/lib9/strecpy.c
>  M src/lib9/tokenize.c
>  M src/lib9/utf/rune.c
>  M src/lib9/utf/utfecpy.c
>  M src/lib9/utf/utflen.c
>  M src/lib9/utf/utfnlen.c
>  M src/lib9/utf/utfrrune.c
>  M src/lib9/utf/utfrune.c
>  M src/lib9/utf/utfutf.c
> 
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
> 

[rsc, 2013-06-21 17:34:34]

I wonder if it makes sense to enable -Wconversion just on a whitelist of
directories. I would start with the higher-level code (cmd/*) in that case. lib9
has not been a source of problems.

[iant, 2013-06-25 05:46:19]

Hello golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2013-06-25 17:37:30]

i'm fine with this, but its status is unclear to me.

[iant, 2013-06-25 17:40:11]

On Tue, Jun 25, 2013 at 10:37 AM,  <r@golang.org> wrote:
> i'm fine with this, but its status is unclear to me.

As far as I'm concerned this is ready to go in.

[r, 2013-06-25 17:43:07]

LGTM

[iant, 2013-06-25 17:44:32]

*** Submitted as https://code.google.com/p/go/source/detail?r=3899cce24466 ***

lib9: avoid all -Wconversion warnings

Built after adding -Wconversion to the list of compiler
arguments used when building.  I believe these are all OK
assuming we will not change the API.  There is no effort to
detect overflow due to very long strings.

R=golang-dev, dave, rsc, r
CC=golang-dev
https://codereview.appspot.com/10195044

[jnj, 2013-06-26 01:16:31]

On 2013/06/25 17:44:32, iant wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=3899cce24466 ***
> 
> lib9: avoid all -Wconversion warnings
> 
> Built after adding -Wconversion to the list of compiler
> arguments used when building.  I believe these are all OK
> assuming we will not change the API.  There is no effort to
> detect overflow due to very long strings.
> 
> R=golang-dev, dave, rsc, r
> CC=golang-dev
> https://codereview.appspot.com/10195044

Hello, this revision seems to break the plan9 build:

# Building C bootstrap tool.
cmd/dist

# Building compilers and Go bootstrap tool for host, plan9/386.
lib9
/tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/src/lib9/flag.c:57 syntax
error, last name: namelen
warning: /tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/src/lib9/flag.c:51
param declared and not used: creat
warning: /tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/src/lib9/flag.c:56
unreachable FOR inc
/tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/src/lib9/flag.c:60 no return
at end of function: lookflag
/tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/src/lib9/flag.c:64 syntax
error, last name: if
go tool dist: FAILED: /bin/8c -FTVw
-I/tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/include/plan9
-I/tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/include/plan9/386 -I
/tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/src/lib9 -o $WORK/flag.8
/tmp/gobuilder/plan9-386-cnielsen-3899cce24466/go/src/lib9/flag.c:
'.../gobuilder/plan9-386-cnielsen-3899cce24466/go/pkg/obj/plan9_386/lib9.a' does
not exist
Build complete, duration 16.242767877s. Result: error: exit status: 'all.rc
88402: dist 88453: FAILED: %s'

[bradfitz, 2013-06-26 01:22:30]

The plan9 build has always been broken: http://build.golang.org/
On Jun 25, 2013 6:16 PM, <jeremyjackins@gmail.com> wrote:

> On 2013/06/25 17:44:32, iant wrote:
>
>> *** Submitted as
>>
>
https://code.google.com/p/go/**source/detail?r=3899cce24466<https://code.goog...
>
>  lib9: avoid all -Wconversion warnings
>>
>
>  Built after adding -Wconversion to the list of compiler
>> arguments used when building.  I believe these are all OK
>> assuming we will not change the API.  There is no effort to
>> detect overflow due to very long strings.
>>
>
>  R=golang-dev, dave, rsc, r
>> CC=golang-dev
>>
https://codereview.appspot.**com/10195044<https://codereview.appspot.com/1019...
>>
>
> Hello, this revision seems to break the plan9 build:
>
> # Building C bootstrap tool.
> cmd/dist
>
> # Building compilers and Go bootstrap tool for host, plan9/386.
> lib9
> /tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/src/**lib9/flag.c:57
> syntax error, last name: namelen
> warning:
> /tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/src/**lib9/flag.c:51
> param declared and not used: creat
> warning:
> /tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/src/**lib9/flag.c:56
> unreachable FOR inc
> /tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/src/**lib9/flag.c:60
> no
> return at end of function: lookflag
> /tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/src/**lib9/flag.c:64
> syntax error, last name: if
> go tool dist: FAILED: /bin/8c -FTVw
> -I/tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/**include/plan9
> -I/tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/**include/plan9/386
> -I
> /tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/src/**lib9 -o
> $WORK/flag.8
> /tmp/gobuilder/plan9-386-**cnielsen-3899cce24466/go/src/**lib9/flag.c:
> '.../gobuilder/plan9-386-**cnielsen-3899cce24466/go/pkg/**
> obj/plan9_386/lib9.a'
> does not exist
> Build complete, duration 16.242767877s. Result: error: exit status:
> 'all.rc 88402: dist 88453: FAILED: %s'
>
>
https://codereview.appspot.**com/10195044/<https://codereview.appspot.com/101...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[iant, 2013-06-26 05:08:46]

On Tue, Jun 25, 2013 at 6:16 PM,  <jeremyjackins@gmail.com> wrote:
>
> Hello, this revision seems to break the plan9 build:

It looks like the Plan 9 compiler is missing a definition of size_t.
I'm happy to review a CL but I don't have a way to test it.

Ian

[rsc, 2013-06-26 05:09:18]

The Plan 9 C libraries do not use size_t anywhere.

Russ

[iant, 2013-06-26 05:11:54]

On Tue, Jun 25, 2013 at 10:09 PM, Russ Cox <rsc@golang.org> wrote:
> The Plan 9 C libraries do not use size_t anywhere.

Any suggestions as to a better fix?  The goal of the CL was to
eliminate -Wconversion warnings.  The casts to size_t are added for
things like calls to memcmp.  The last argument to memcmp is a size_t
on most libraries, and -Wconversion triggers a warning when we pass an
int, as it should.

What is the type of the third argument to memcmp in the Plan 9 libraries?

Ian

[rsc, 2013-06-26 05:22:03]

It's unsigned long (
http://plan9.bell-labs.com/sources/plan9/sys/include/libc.h).
You might be able to get away with adding a typedef of size_t to
$GOROOT/include/plan9/libc.h.

[aram, 2013-06-26 11:24:41]

Brad Fitzpatrick wrote:
> The plan9 build has always been broken: http://build.golang.org/

Not really. The tests just timed out (machine too slow?). Now the build 
really is broken.

Why is the Plan 9 builder so slow. Who hosts it, what hardware, is it 
virtual?