code review 106860045: build: annotations and modifications for c2go

build: annotations and modifications for c2go

The main changes fall into a few patterns:

1. Replace #define with enum.

2. Add /*c2go */ comment giving effect of #define.
This is necessary for function-like #defines and
non-enum-able #defined constants.
(Not all compilers handle negative or large enums.)

3. Add extra braces in struct initializer.
(c2go does not implement the full rules.)

This is enough to let c2go typecheck the source tree.
There may be more changes once it is doing
other semantic analyses.

[rsc, 2014-07-01 13:17:40]

Hello minux (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[dave_cheney.net, 2014-07-01 13:46:31]

Build passes on linux/arm and darwin/amd64

https://codereview.appspot.com/106860045/diff/20001/src/cmd/5l/5.out.h
File src/cmd/5l/5.out.h (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/5l/5.out.h#newcode52
src/cmd/5l/5.out.h:52: REGM = REGEXT-1,
REGM is no more

https://codereview.appspot.com/106860045/diff/20001/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/gc/subr.c#newcode...
src/cmd/gc/subr.c:3445: mask = 0xffffffffLL;
should this be 0xffffffffUL ?

https://codereview.appspot.com/106860045/diff/20001/src/cmd/ld/textflag.h
File src/cmd/ld/textflag.h (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/ld/textflag.h#new...
src/cmd/ld/textflag.h:28: NOPROF = 1,
does c2go transfer the comments from the #defines to their enum counterparts ?

https://codereview.appspot.com/106860045/diff/20001/src/liblink/ld.c
File src/liblink/ld.c (right):

https://codereview.appspot.com/106860045/diff/20001/src/liblink/ld.c#newcode165
src/liblink/ld.c:165: {
this magic number needs a comment please.

[rsc, 2014-07-01 15:14:29]

https://codereview.appspot.com/106860045/diff/20001/src/cmd/5l/5.out.h
File src/cmd/5l/5.out.h (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/5l/5.out.h#newcode52
src/cmd/5l/5.out.h:52: REGM = REGEXT-1,
On 2014/07/01 13:46:31, dfc wrote:
> REGM is no more

I noticed that too but I didn't want to change the code semantically here.

https://codereview.appspot.com/106860045/diff/20001/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/gc/subr.c#newcode...
src/cmd/gc/subr.c:3445: mask = 0xffffffffLL;
On 2014/07/01 13:46:31, dfc wrote:
> should this be 0xffffffffUL ?

it doesn't matter - they both mean the same number when converted to uint64.
the problem with the case 64 constant is that 0xffffffffffffffff does not fit in
an LL.

https://codereview.appspot.com/106860045/diff/20001/src/cmd/ld/textflag.h
File src/cmd/ld/textflag.h (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/ld/textflag.h#new...
src/cmd/ld/textflag.h:28: NOPROF = 1,
On 2014/07/01 13:46:31, dfc wrote:
> does c2go transfer the comments from the #defines to their enum counterparts ?

no. it ignores #defines completely. there will be more passes for cleanup like
that. for now i'm worrying about just getting it to accept the code.

https://codereview.appspot.com/106860045/diff/20001/src/liblink/ld.c
File src/liblink/ld.c (right):

https://codereview.appspot.com/106860045/diff/20001/src/liblink/ld.c#newcode165
src/liblink/ld.c:165: {
On 2014/07/01 13:46:31, dfc wrote:
> this magic number needs a comment please.

later. this is only about making the code readable by c2go, not about cleanup.
(it's the log base and max height for the skip list.)

[bradfitz, 2014-07-01 16:06:11]

https://codereview.appspot.com/106860045/diff/20001/include/link.h
File include/link.h (right):

https://codereview.appspot.com/106860045/diff/20001/include/link.h#newcode172
include/link.h:172: int	np;
what was this one for?

[minux, 2014-07-01 17:48:58]

https://codereview.appspot.com/106860045/diff/20001/src/cmd/8l/8.out.h
File src/cmd/8l/8.out.h (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/8l/8.out.h#newcod...
src/cmd/8l/8.out.h:676: #define	SYMDEF	"__.GOSYMDEF"
so this one doesn't need
/*c2go extern char SYMDEF[]; */
?

https://codereview.appspot.com/106860045/diff/20001/src/cmd/gc/plive.c
File src/cmd/gc/plive.c (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/gc/plive.c#newcod...
src/cmd/gc/plive.c:660: Addr *from;
do we still need the #define Adr Addr
in [568]g/opt.h?

https://codereview.appspot.com/106860045/diff/20001/src/liblink/obj5.c
File src/liblink/obj5.c (right):

https://codereview.appspot.com/106860045/diff/20001/src/liblink/obj5.c#newcode67
src/liblink/obj5.c:67: iscall5(Prog *p)
why this change? it's static, so no name clash is possible.

[rsc, 2014-07-01 18:13:20]

PTAL

https://codereview.appspot.com/106860045/diff/20001/include/link.h
File include/link.h (right):

https://codereview.appspot.com/106860045/diff/20001/include/link.h#newcode172
include/link.h:172: int	np;
On 2014/07/01 16:06:10, bradfitz wrote:
> what was this one for?

not sure. doesn't matter, but reverted.

https://codereview.appspot.com/106860045/diff/20001/src/cmd/8l/8.out.h
File src/cmd/8l/8.out.h (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/8l/8.out.h#newcod...
src/cmd/8l/8.out.h:676: #define	SYMDEF	"__.GOSYMDEF"
On 2014/07/01 17:48:58, minux wrote:
> so this one doesn't need
> /*c2go extern char SYMDEF[]; */
> ?

no, because nothing i am converting uses SYMDEF

https://codereview.appspot.com/106860045/diff/20001/src/cmd/gc/plive.c
File src/cmd/gc/plive.c (right):

https://codereview.appspot.com/106860045/diff/20001/src/cmd/gc/plive.c#newcod...
src/cmd/gc/plive.c:660: Addr *from;
On 2014/07/01 17:48:58, minux wrote:
> do we still need the #define Adr Addr
> in [568]g/opt.h?

Yes, I stopped before I converted them all.

https://codereview.appspot.com/106860045/diff/20001/src/liblink/obj5.c
File src/liblink/obj5.c (right):

https://codereview.appspot.com/106860045/diff/20001/src/liblink/obj5.c#newcode67
src/liblink/obj5.c:67: iscall5(Prog *p)
On 2014/07/01 17:48:58, minux wrote:
> why this change? it's static, so no name clash is possible.

already reverted.

[minux, 2014-07-01 18:48:04]

LGTM.

[rsc, 2014-07-01 19:36:10]

https://codereview.appspot.com/106860045/diff/20001/include/link.h
File include/link.h (right):

https://codereview.appspot.com/106860045/diff/20001/include/link.h#newcode172
include/link.h:172: int	np;
now i remember.
liblink/objfile.c contains
	rddata(f, &s->p, &s->np);
but rddata is written to take an int* not an int32*.
they might be different types, and c2go treats them as such.

[iant, 2014-07-01 22:22:57]

LGTM

https://codereview.appspot.com/106860045/diff/60001/src/cmd/5g/opt.h
File src/cmd/5g/opt.h (right):

https://codereview.appspot.com/106860045/diff/60001/src/cmd/5g/opt.h#newcode64
src/cmd/5g/opt.h:64: uint32 BLOAD(Reg*);
As far as I can see nothing calls BLOAD and BSTORE anyhow.  Which is good,
because these declarations look wrong.  BLOAD and BSTORE return Bits, not
uint32.

https://codereview.appspot.com/106860045/diff/60001/src/cmd/6g/opt.h
File src/cmd/6g/opt.h (right):

https://codereview.appspot.com/106860045/diff/60001/src/cmd/6g/opt.h#newcode64
src/cmd/6g/opt.h:64: uint32 BLOAD(Reg*);
BLOAD and BSTORE not needed and incorrect.

https://codereview.appspot.com/106860045/diff/60001/src/cmd/8g/opt.h
File src/cmd/8g/opt.h (right):

https://codereview.appspot.com/106860045/diff/60001/src/cmd/8g/opt.h#newcode64
src/cmd/8g/opt.h:64: uint32 BLOAD(Reg*);
BLOAD and BSTORE not needed and incorrect.

[rsc, 2014-07-02 19:41:32]

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

build: annotations and modifications for c2go

The main changes fall into a few patterns:

1. Replace #define with enum.

2. Add /*c2go */ comment giving effect of #define.
This is necessary for function-like #defines and
non-enum-able #defined constants.
(Not all compilers handle negative or large enums.)

3. Add extra braces in struct initializer.
(c2go does not implement the full rules.)

This is enough to let c2go typecheck the source tree.
There may be more changes once it is doing
other semantic analyses.

LGTM=minux, iant
R=minux, dave, iant
CC=golang-codereviews
https://codereview.appspot.com/106860045

[gobot, 2014-07-02 19:57:56]

This CL appears to have broken the windows-amd64-race builder.
See http://build.golang.org/log/f2272da8d4240bb745e707f39a72455f7407c61c