code review 7357046: go/types: support for customizable Alignof, Sizeof

go/types: support for customizable Alignof, Sizeof

(Offsetof is a function of Alignof and Sizeof.)

- removed IntSize, PtrSize from Context (set Sizeof instead)
- GcImporter needs a Context now (it needs to have
  access to Sizeof/Alignof)
- removed exported Size field from Basic (use Sizeof)
- added Offset to Field
- added Alignment, Size to Struct

[gri, 2013-02-20 07:32:58]

Hello adonovan@google.com (cc: golang-dev@googlegroups.com),

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

[adonovan, 2013-02-20 15:41:22]

LGTM

https://codereview.appspot.com/7357046/diff/6002/src/pkg/exp/ssa/builder.go
File src/pkg/exp/ssa/builder.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/exp/ssa/builder.go#n...
src/pkg/exp/ssa/builder.go:165: // TODO(adonovan): permit the client to specify
these
This comment is now orphaned; please delete.

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/api.go
File src/pkg/go/types/api.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/api.go#newc...
src/pkg/go/types/api.go:83: // ast.Objects. If there are no errors, Check
returns the package, otherwise
Since you're here:
"assigning types to ast.Objects": is this still true?

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go
File src/pkg/go/types/builtins.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go...
src/pkg/go/types/builtins.go:325: case _Offsetof:
Curious: when this is so clearly a special form, why do we try to make it look
it library function?  Also, I can see that using it is potentially nonportable,
but is it really unsafe?

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go...
src/pkg/go/types/builtins.go:524: return DefaultPtrSize * 2
These constants seem reasonable but what's the rationale for their specific
values?  Are they identical to gc?  (If so, document.)

Is it now the case that these three functions are now identical to those of gc
for some values of {Int,Ptr}Size?

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/expr.go
File src/pkg/go/types/expr.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/expr.go#new...
src/pkg/go/types/expr.go:169: if r := x % a; r > 0 {
Or branch-free like so:
        y := x + a - 1
	return y - y % a

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:156: // gcParser parses the exports inside a gc
compiler-produced
Does a gc object file encode its sizeof/alignof parameters in any form?

If ABI compatibility ever becomes an issue (perhaps that's a stretch), you'd
want to be able to check this.

[gri, 2013-02-20 18:16:13]

https://codereview.appspot.com/7357046/diff/6002/src/pkg/exp/ssa/builder.go
File src/pkg/exp/ssa/builder.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/exp/ssa/builder.go#n...
src/pkg/exp/ssa/builder.go:165: // TODO(adonovan): permit the client to specify
these
On 2013/02/20 15:41:22, adonovan wrote:
> This comment is now orphaned; please delete.

Done.

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/api.go
File src/pkg/go/types/api.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/api.go#newc...
src/pkg/go/types/api.go:83: // ast.Objects. If there are no errors, Check
returns the package, otherwise
On 2013/02/20 15:41:22, adonovan wrote:
> Since you're here:
> "assigning types to ast.Objects": is this still true?
no. fixed.

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go
File src/pkg/go/types/builtins.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go...
src/pkg/go/types/builtins.go:325: case _Offsetof:
On 2013/02/20 15:41:22, adonovan wrote:
> Curious: when this is so clearly a special form, why do we try to make it look
> it library function?  Also, I can see that using it is potentially
nonportable,
> but is it really unsafe?

Built-in functions are the mechanism we have in Go for special situations. It's
non-portable, and perhaps not unsafe, but still, the very same program using
unsafe.Offsetof may compile on one platform, and not on the other (say, the
result constant could be used in a expression that results in a div-zero compile
time error dep. on offset value). Package unsafe seems as good as any other
special case, and unsafe we have.

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go...
src/pkg/go/types/builtins.go:524: return DefaultPtrSize * 2
On 2013/02/20 15:41:22, adonovan wrote:
> These constants seem reasonable but what's the rationale for their specific
> values?  Are they identical to gc?  (If so, document.)
> 
> Is it now the case that these three functions are now identical to those of gc
> for some values of {Int,Ptr}Size?

They are just reasonable. I am not claiming anything else. Having reasonable
values permits reasonable programs to typecheck reasonably, with default
context.

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/expr.go
File src/pkg/go/types/expr.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/expr.go#new...
src/pkg/go/types/expr.go:169: if r := x % a; r > 0 {
On 2013/02/20 15:41:22, adonovan wrote:
> Or branch-free like so:
>         y := x + a - 1
> 	return y - y % a

yes, thanks.

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:156: // gcParser parses the exports inside a gc
compiler-produced
On 2013/02/20 15:41:22, adonovan wrote:
> Does a gc object file encode its sizeof/alignof parameters in any form?
> 
> If ABI compatibility ever becomes an issue (perhaps that's a stretch), you'd
> want to be able to check this.

Not that I know of. The offsets may be encoded elsewhere, but not in the export
data.

[gri, 2013-02-20 19:10:22]

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

go/types: support for customizable Alignof, Sizeof

(Offsetof is a function of Alignof and Sizeof.)

- removed IntSize, PtrSize from Context (set Sizeof instead)
- GcImporter needs a Context now (it needs to have
  access to Sizeof/Alignof)
- removed exported Size field from Basic (use Sizeof)
- added Offset to Field
- added Alignment, Size to Struct

R=adonovan
CC=golang-dev
https://codereview.appspot.com/7357046

[adonovan, 2013-05-09 21:53:58]

LGTM

(pair-reviewed)

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go
File src/pkg/go/types/builtins.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go...
src/pkg/go/types/builtins.go:325: case _Offsetof:
On 2013/02/20 18:16:13, gri wrote:
> On 2013/02/20 15:41:22, adonovan wrote:
> > Curious: when this is so clearly a special form, why do we try to make it
look
> > it library function?  Also, I can see that using it is potentially
> nonportable,
> > but is it really unsafe?
> 
> Built-in functions are the mechanism we have in Go for special situations.
It's
> non-portable, and perhaps not unsafe, but still, the very same program using
> unsafe.Offsetof may compile on one platform, and not on the other (say, the
> result constant could be used in a expression that results in a div-zero
compile
> time error dep. on offset value). Package unsafe seems as good as any other
> special case, and unsafe we have.

Well, I can determine sizeof(x) even in the supposedly portable (safe) parts of
the language. :)  

My point was that the operands to sizeof, offsetof and alignof are not values
but types (or struct/fieldname pairs) so they are even more special than append,
which could in principle have a (polymorphic) type in some hypothetical future
release.  So these things need compiler support, yet are not in the global
namespace.   

Is putting them in "unsafe" a namespace pollution issue?

[gri, 2013-05-09 22:05:21]

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go
File src/pkg/go/types/builtins.go (right):

https://codereview.appspot.com/7357046/diff/6002/src/pkg/go/types/builtins.go...
src/pkg/go/types/builtins.go:325: case _Offsetof:
On 2013/05/09 21:53:58, adonovan wrote:
> On 2013/02/20 18:16:13, gri wrote:
> > On 2013/02/20 15:41:22, adonovan wrote:
> > > Curious: when this is so clearly a special form, why do we try to make it
> look
> > > it library function?  Also, I can see that using it is potentially
> > nonportable,
> > > but is it really unsafe?
> > 
> > Built-in functions are the mechanism we have in Go for special situations.
> It's
> > non-portable, and perhaps not unsafe, but still, the very same program using
> > unsafe.Offsetof may compile on one platform, and not on the other (say, the
> > result constant could be used in a expression that results in a div-zero
> compile
> > time error dep. on offset value). Package unsafe seems as good as any other
> > special case, and unsafe we have.
> 
> Well, I can determine sizeof(x) even in the supposedly portable (safe) parts
of
> the language. :)  
> 
> My point was that the operands to sizeof, offsetof and alignof are not values
> but types (or struct/fieldname pairs) so they are even more special than
append,
> which could in principle have a (polymorphic) type in some hypothetical future
> release.  So these things need compiler support, yet are not in the global
> namespace.   
> 
> Is putting them in "unsafe" a namespace pollution issue?

Not really. It's just a design decision. In some sense, all the size operations
are "safe" but at the same time they are (likely) not often used unless in
conjunction with unsafe pointer operations. Arguably, it would be nice to have
sizeof w/o the need to import unsafe. One argument one might make that it
exposes implementation details and thus makes code non-portable (I am aware that
this is not a very sharp argument).