code review 4314054: go/types: New Go type hierarchy implementation for AST.

go/types: New Go type hierarchy implementation for AST.

This CL defines a new, more Go-like representation of
Go types (different structs for different types as
opposed to a single Type node). It also implements
an ast.Importer for object/archive files generated
by the gc compiler tool chain. Besides the individual
type structs, the main difference is the handling of
named types: In the old world, a named type had a
non-nil *Object pointer but otherwise looked no
different from other types. In this new model, named
types have their own representation types.Name. As
a result, resolving cycles is a bit simpler during
construction, at the cost of having to deal with
types.Name nodes explicitly later. It remains to be
seen if this is a good approach. Nevertheless, code
involving types reads more nicely and benefits from
full type checking. Also, the representation seems
to more closely match the spec wording.

Credits: The original version of the gc importer was
written by Evan Shaw (chickencha@gmail.com). The new
version in this CL is based largely on Evan's original
code but contains bug fixes, a few simplifications,
some restructuring, and was adjusted to use the
new type hierarchy. I have added a comprehensive test
that imports all packages found under $GOROOT/pkg (with
a 3s time-out to limit the run-time of the test). Run
gotest -v for details.

The original version of ExportData (exportdata.go) was
written by Russ Cox (rsc@golang.org). The current version
is returning the internal buffer positioned at the beginning
of the export data instead of printing the export data to
stdout.

With the new types package, the existing in-progress
typechecker package is deprecated. I will delete it
once all functionality has been brought over.

[gri, 2011-04-06 01:30:39]

Hello golang-dev@googlegroups.com,

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

[gri, 2011-04-06 01:45:41]

hg mail missed most of the CL: Here is what I have:

4314054:
	go/types: New Go type hierarchy implementation for AST.
	
	This CL defines a new, more Go-like representation of
	Go types (different structs for different types as
	opposed to a single Type node). It also implements
	an ast.Importer for object/archive files generated
	by the gc compiler tool chain. Besides the individual
	type structs, the main difference is the handling of
	named types: In the old world, a named type had a
	non-nil *Object pointer but otherwise looked no
	different from other types. In this new model, named
	types have their own representation types.Name. As
	a result, resolving cycles is a bit simpler during
	construction, at the cost of having to deal with
	types.Name nodes explicitly later. It remains to be
	seen if this is a good approach. Nevertheless, code
	involving types reads more nicely and benefits from
	full type checking. Also, the representation seems
	to more closely match the spec wording.
	
	Credits: The original version of the gc importer was
	written by Evan Shaw (chickencha@gmail.com). The new
	version in this CL is based largely on Evan's original
	code but contains bug fixes, several simplifications,
	significant restructuring, and was adjusted to use the
	new type hierarchy. I have added a comprehensive test
	that imports all packages found under $GOROOT/pkg (with
	a 3s time-out to limit the run-time of the test). Run
	gotest -v for details.
	
	With the new types package, the existing in-progress
	typechecker package is deprecated. I will delete it
	once all functionality has been brought over.

	Author: gri@golang.org
	Reviewer:
	CC:
	Files:
		src/pkg/Makefile
		src/pkg/go/types/Makefile
		src/pkg/go/types/const.go
		src/pkg/go/types/gcimporter.go
		src/pkg/go/types/gcimporter_test.go
		src/pkg/go/types/testdata/exports.go
		src/pkg/go/types/types.go
		src/pkg/go/types/universe.go
		src/pkg/go/typechecker/typechecker.go

[eds, 2011-04-06 02:04:46]

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go
File src/pkg/go/types/const.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:44: func NewConst(kind token.Token, lit string) Const
{
There should be a way to get a boolean Const from this function. I'm not sure
what token you'd use (token.IDENT?).

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/universe.go
File src/pkg/go/types/universe.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/universe.go...
src/pkg/go/types/universe.go:65: defType("uint8")
Seems like something more is needed to make byte and uint8 aliases. Am I missing
something?

[rog, 2011-04-06 08:35:24]

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go
File src/pkg/go/types/const.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:100: switch b := y.val.(type) {
switch y.val.(type) {

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:113: switch b := y.val.(type) {
switch y.val.(type) {

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:122: switch b := y.val.(type) {
switch y.val.(type) {

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:53: func seek(buf *bufio.Reader, s string)
os.Error {
i think i'd call this "find" or "search", as seek has other connotations when
reading.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:64: if string(b) == s {
using bytes.Equal would be a quick way to speed things up a bit.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:370: scope.Insert(par)
this won't work correctly for unnamed parameters AFAICS

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/types.go
File src/pkg/go/types/types.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:67: Params *ast.Scope // always present
How is the order of the function arguments preserved, given that a Scope is an
unordered map? (same question for Struct above) And what about the return
type(s) ?

[gri, 2011-04-07 00:05:15]

PTAL.
- gri

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go
File src/pkg/go/types/const.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:44: func NewConst(kind token.Token, lit string) Const
{
On 2011/04/06 02:04:46, eds wrote:
> There should be a way to get a boolean Const from this function. I'm not sure
> what token you'd use (token.IDENT?).

Not convinced about this. NewConst is intended to create constants from literals
only. true and false are idents predeclared in the universe and they already
have values - they are not literals. NewConst will not be called for them when
type checking the AST.

The use in gcimporter is just convenience. Leave as is.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:100: switch b := y.val.(type) {
On 2011/04/06 08:35:25, rog wrote:
> switch y.val.(type) {

Done.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:113: switch b := y.val.(type) {
On 2011/04/06 08:35:25, rog wrote:
> switch y.val.(type) {

Done.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:122: switch b := y.val.(type) {
On 2011/04/06 08:35:25, rog wrote:
> switch y.val.(type) {

Done.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:53: func seek(buf *bufio.Reader, s string)
os.Error {
On 2011/04/06 08:35:25, rog wrote:
> i think i'd call this "find" or "search", as seek has other connotations when
> reading.

Done.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:64: if string(b) == s {
On 2011/04/06 08:35:25, rog wrote:
> using bytes.Equal would be a quick way to speed things up a bit.

Done.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:370: scope.Insert(par)
On 2011/04/06 08:35:25, rog wrote:
> this won't work correctly for unnamed parameters AFAICS

changed parseName() to return "_" in that case
(still an issue with anonymous fields, but there's a TODO already)

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/types.go
File src/pkg/go/types/types.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:67: Params *ast.Scope // always present
On 2011/04/06 08:35:25, rog wrote:
> How is the order of the function arguments preserved, given that a Scope is an
> unordered map? (same question for Struct above) And what about the return
> type(s) ?

I am aware that this needs to change. I didn't want to define a particular
layout (say, Recv, Ins, Outs) before writing the code that is actually using it
(since that code may influence the layout).

This CL is about getting the importer in. I'm sure there will be fine-tuning of
the Type types.

Added TODO.

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/universe.go
File src/pkg/go/types/universe.go (right):

http://codereview.appspot.com/4314054/diff/27001/src/pkg/go/types/universe.go...
src/pkg/go/types/universe.go:65: defType("uint8")
On 2011/04/06 02:04:46, eds wrote:
> Seems like something more is needed to make byte and uint8 aliases. Am I
missing
> something?

yes, you're correct. not sure yet what the best way of doing this is (the
easiest might be just a special case when comparing types). added TODO.

[eds, 2011-04-07 01:09:26]

LGTM

On Thu, Apr 7, 2011 at 12:05 PM,  <gri@golang.org> wrote:
>> There should be a way to get a boolean Const from this function. I'm
>
> not sure
>>
>> what token you'd use (token.IDENT?).
>
> Not convinced about this. NewConst is intended to create constants from
> literals only. true and false are idents predeclared in the universe and
> they already have values - they are not literals. NewConst will not be
> called for them when type checking the AST.
>
> The use in gcimporter is just convenience. Leave as is.

Fair enough. It just seems strange that there's no direct way to
create a boolean constant from outside this package when there's a way
to create one for every other basic type. I admit I don't have a use
case in mind, though.

- Evan

[rsc, 2011-04-07 16:27:19]

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/const.go
File src/pkg/go/types/const.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:76: // NewZero creates a new zero constant for the
given type.
Since it doesn't return a pointer, New is a little confusing.

Zero?
ZeroConst?

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:83: // IsValid returns true if x represents a constant
value.
add?
// That is, IsValid returns true if x is not the zero Const.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:93: func (x Const) Match(y Const) (u, v Const) {
All of these methods on Const start with a 
switch on x.val.(type).  Is there a reason
not to make Const itself an interface and
make these methods on the sub-pieces?

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:5: // This file implements an ast.Importer for gc
generated object files.
s/gc generated/gc-generated/

It might make sense to move this into a different CL,
so that one is about types and one is about the import
metadata details.  I don't mind keeping it one big CL
though if you'd prefer that.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:33: // importFilename returns the filename for an
import path
There are really two outputs from this search:
the file to use, and the string that will identify
it in the import data.  The cases are:

"x" -> "$GOROOT/pkg/$GOOS_$GOARCH/x.a", "x"
"./x" -> "/this/directory/x.a", "/this/directory/x"
"/x" -> "/x.a", "/x"

Notice that the relative path becomes an absolute
path in the import data, in case the object referring
to it moves.

This is particularly important when you see the
"" import path.  The second return value in the examples
above is what it should be replaced with. 

On that topic, I am a bit confused about the fact that
the GcImporter seems to have no state shared across
invocations.  If you have

package p

type T struct { sync.Mutex }

package q

type T struct { *sync.Mutex }

package main

import "p"
import "q"

var pt p.T
var qt = q.T{&pt.Mutex}

That can't be type checked unless somehow the two definitions
of "sync".Mutex found in the two package metadata sets get
merged to a single type.  Is that just future work?

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:53: // TODO(gri) consider putting a good
implementation of find into bufio.
Please don't.  The problem with this function is that it
assumes a buffer size.  The use is wrong anyway, so it's
not necessary.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:114: if !trace {
This looks inconsequential on first glance
but it's very important.  Maybe drop this if
(but keep the body) and move the conditional
part into the deferred func.

err = r.(importError)
if trace {
    panic(err)  // for stack trace
}

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:138: // allocate a buffer and advance it to the
beginning of the export section
This is not the right algorithm.  The // exports comment
only occurs in .6 files, not in the export data for .a files.
This code is working because it is skipping past the real
.a export data and pulling it out of the _go_.6 file inside
the archive.  But that means the compiler and this code are
looking at different data.  I will send a separate reply with
the right code.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:166: return fmt.Sprintf("import error %s (offs =
%d): %s", e.pos, e.pos.Offset, e.err)
s/offs =/at byte offset/
offs sounds plural

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:225: // ExportedName = ImportPath "." identifier
[ "·" [ int_lit ] ] .
The real syntax is

ExportedName = ImportPath "." identifier

where identifier is the Go definition plus · characters.
It would be more robust to make the identifier scanner
accept that definition.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:286: // "[" already consumed
// "[" already consumed and lookahead known not to be "]"

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:290: n, err := strconv.Atoui64(lit)
Should this be using the Const routines instead?

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:313: name := "_"
name := ""

As written the code conflates ? and _, but they are not the same.

type T struct {
    _ sync.Mutex
}

type T struct {
    sync.Mutex  // import metadata says "? sync.Mutex"
}

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:431: // InterfaceType = "interface" "{" {
MethodSpec } "}" .
missing the ";"

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:452: // for simplicity we accept: [ "<-" ] chan [
"<-" ] Type .
Doesn't this get the precedence wrong?
See if you can import the result of compiling test/import2.go.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:513: return &Slice{Ellipsis: true, Elt:
p.parseType()}
This is blurring syntax and semantics (see comment in other file).
An alternative is to make the function parameter parser look for the ...

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:533: // If the ImportDecl were using the same
notation as all other
The exported name is a selector expression.
An import path is not.  I don't understand why
they would use the same syntax.  It's true that
they are both an import path and a name,
but they have different meanings.

Delete comment?

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:536: p.expect(scanner.Ident) // throw away for
now
// The identifier has no semantic meaning in the import data.
// It exists so that error messages can print the real package
// name: binary.ByteOrder instead of "encoding/binary".ByteOrder.
// TODO(gri): Save import path -> package name mapping.
p.expect(scanner.Ident)

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:565: return NewConst(token.FLOAT, mant+"e"+exp)
3p5 means 3*2^5 = 96, but this rewrites it to 3e5 = 300000.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:593: typ = Bool.Underlying
This is confusing.  Maybe there should be top-level names
IdealBool, IdealComplex, IdealInt, IdealFloat, IdealString?

Also this should only set if typ == nil.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:633: name.Underlying = Underlying(typ)
It should not be possible for typ to be a named type here,
so the call to Underlying should be redundant.  Fine though.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:670: // Decl = [ ImportDecl | ConstDecl| TypeDecl
| VarDecl | FuncDecl | MethodDecl ] "\n" .
s/ConstDecl/& /

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/testdata/ex...
File src/pkg/go/types/testdata/exports.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/testdata/ex...
src/pkg/go/types/testdata/exports.go:33: T6 chan<- int
Worth adding
   chan (<-chan int)
   chan<- (chan int)

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go
File src/pkg/go/types/types.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:13: // All types implement the Type interface.
Is the intent to expand this?  If so would the isType()
method then become redundant?

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:45: Ellipsis bool // true for "...Elt" types
The ... is a property of the function type, not a slice type.
I think it's a mistake to stick it here, because any code
that wants to deal with slices is going to have to look at it
and think "hmm, what's going on with this?".  I suggest adding

IsVariadic bool

to the Func instead.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:53: Fields *ast.Scope // always present
I'm surprised to see the ast mixed in here.
I think it would be clearer to separate them
completely, introducing a type Field and making
this []*Field.  It makes sense for the type tree
to have references back to the original ast, but
this representation implies that the original
ast then has references back to this package,
because otherwise you couldn't find out the
type of a field.  I think you'll have more flexibility
if they're not so tightly coupled, the same way we
have more flexibility when we avoid cyclic dependencies.

If this is just a placeholder for something else later,
I'd suggest leaving it out for now to make sure it
doesn't get depended on.

Same applies to the other uses of *ast.Scope here.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:62: Base Type
can this be Elt?

Reflect uses the same name for all of these (Elem)
and it would be nice if the mental translation was
1:1 (Elem -> Elt).

It would be even nicer if this package used the same
speling as reflect (Ptr, Elem) but I'm not going to
push for that.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:69: Params *ast.Scope // always present
This is even more surprising as a scope, since
the names involved are not relevant.  I expected
something like

In []Type
Out []Type
IsVariadic bool

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:78: Methods *ast.Scope // always present
Again.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:102: Scope      *ast.Scope  // associated methods (and
fields, if any); or nil
Again.

[rsc, 2011-04-07 16:28:32]

Code for finding the import data.

package main

import (
	"bufio"
	"fmt"
	"io"
	"log"
	"os"
	"strconv"
	"strings"
)

func read(filename string) (err os.Error) {
	f, err := os.Open(filename)
	if err != nil {
		return
	}
	
	defer func() {
		// Add file name to error.
		if err != nil {
			err = fmt.Errorf("reading %s: %v", filename, err)
		}
	}()

	b := bufio.NewReader(f)
	
	// Read first line to make sure this is an object file.
	line, err := b.ReadSlice('\n')
	if err != nil {
		return
	}
	if string(line) == "!<arch>\n" {
		// Archive file.  Scan to __.PKGDEF, which should
		// be second archive entry.
		var name string
		var size int
		
		// First entry should be __.SYMDEF.
		// Read and discard.
		if name, size, err = readGopackHeader(b); err != nil {
			return
		}
		if name != "__.SYMDEF" {
			err = os.NewError("go archive does not begin with __.SYMDEF")
			return
		}
		const block = 4096
		tmp := make([]byte, block)
		for size > 0 {
			n := size
			if n > block {
				n = block
			}
			_, err = io.ReadFull(b, tmp[:n])
			if err != nil {
				return
			}
			size -= n
		}
		
		// Second entry should be __.PKGDEF.
		if name, size, err = readGopackHeader(b); err != nil {
			return
		}
		if name != "__.PKGDEF" {
			err = os.NewError("go archive is missing __.PKGDEF")
			return
		}
		
		// Read first line of __.PKGDEF data, so that line
		// is once again the first line of the input.
		line, err = b.ReadSlice('\n')
		if err != nil {
			return
		}		
	}

	// Now at __.PKGDEF in archive or still at beginning of file.
	// Either way, line should begin with "go object ".
	if !strings.HasPrefix(string(line), "go object ") {
		err = os.NewError("not a go object file")
		return
	}
	
	// Skip over object header to export data.
	// Begins after first line with $$.
	for line[0] != '$' {
		line, err = b.ReadSlice('\n')
		if err != nil {
			return
		}
	}

	// Dump export data to stdout.
	for {
		line, err = b.ReadSlice('\n')
		if err == os.EOF {
			err = io.ErrUnexpectedEOF
			return
		}
		if err != nil {
			return
		}
		if line[0] == '$' {
			break
		}
		os.Stdout.Write(line)
	}
	return
}

func readGopackHeader(b *bufio.Reader) (name string, size int, err os.Error) {
	// See $GOROOT/include/ar.h.
	hdr := make([]byte, 64+12+6+6+8+10+2)
	_, err = io.ReadFull(b, hdr)
	if err != nil {
		return
	}
	fmt.Printf("header: %s\n", hdr)
	s := strings.TrimSpace(string(hdr[64+12+6+6+8:][:10]))
	size, err = strconv.Atoi(s)
	if err != nil || hdr[len(hdr)-2] != '`' || hdr[len(hdr)-1] != '\n' {
		err = os.NewError("invalid archive header")
		return
	}
	name = strings.TrimSpace(string(hdr[:64]))
	return
}

func main() {
	err := read(os.Args[1])
	if err != nil {
		log.Fatal(err)
	}
}

[gri, 2011-04-08 02:41:32]

PTAL

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/const.go
File src/pkg/go/types/const.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:76: // NewZero creates a new zero constant for the
given type.
On 2011/04/07 16:27:19, rsc wrote:
> Since it doesn't return a pointer, New is a little confusing.
> 
> Zero?
> ZeroConst?

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:83: // IsValid returns true if x represents a constant
value.
On 2011/04/07 16:27:19, rsc wrote:
> add?
> // That is, IsValid returns true if x is not the zero Const.

Removed. Not sure it's needed.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/const.go#ne...
src/pkg/go/types/const.go:93: func (x Const) Match(y Const) (u, v Const) {
On 2011/04/07 16:27:19, rsc wrote:
> All of these methods on Const start with a 
> switch on x.val.(type).  Is there a reason
> not to make Const itself an interface and
> make these methods on the sub-pieces?

More narrow API. But I'll consider it once I start using the ops. Added a TODO.

This is a bit a relict from an earlier version where each object had a
(light-weight) ptr to a Const.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:5: // This file implements an ast.Importer for gc
generated object files.
On 2011/04/07 16:27:19, rsc wrote:
> s/gc generated/gc-generated/
> 
> It might make sense to move this into a different CL,
> so that one is about types and one is about the import
> metadata details.  I don't mind keeping it one big CL
> though if you'd prefer that.

I agree. Let's do this in another round. Added TODO.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:33: // importFilename returns the filename for an
import path
On 2011/04/07 16:27:19, rsc wrote:
> There are really two outputs from this search:
> the file to use, and the string that will identify
> it in the import data.  The cases are:
> 
> "x" -> "$GOROOT/pkg/$GOOS_$GOARCH/x.a", "x"
> "./x" -> "/this/directory/x.a", "/this/directory/x"
> "/x" -> "/x.a", "/x"
> 
> Notice that the relative path becomes an absolute
> path in the import data, in case the object referring
> to it moves.
> 
> This is particularly important when you see the
> "" import path.  The second return value in the examples
> above is what it should be replaced with. 

Done.


> On that topic, I am a bit confused about the fact that
> the GcImporter seems to have no state shared across
> invocations.  If you have
> 
> package p
> 
> type T struct { sync.Mutex }
> 
> package q
> 
> type T struct { *sync.Mutex }
> 
> package main
> 
> import "p"
> import "q"
> 
> var pt p.T
> var qt = q.T{&pt.Mutex}
> 
> That can't be type checked unless somehow the two definitions
> of "sync".Mutex found in the two package metadata sets get
> merged to a single type.  Is that just future work?

Yes. I know that I need a package global table of imported packages (and
respective scopes) eventually. Added TODO.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:53: // TODO(gri) consider putting a good
implementation of find into bufio.
On 2011/04/07 16:27:19, rsc wrote:
> Please don't.  The problem with this function is that it
> assumes a buffer size.  The use is wrong anyway, so it's
> not necessary.
> 

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:114: if !trace {
On 2011/04/07 16:27:19, rsc wrote:
> This looks inconsequential on first glance
> but it's very important.  Maybe drop this if
> (but keep the body) and move the conditional
> part into the deferred func.
> 
> err = r.(importError)
> if trace {
>     panic(err)  // for stack trace
> }

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:138: // allocate a buffer and advance it to the
beginning of the export section
On 2011/04/07 16:27:19, rsc wrote:
> This is not the right algorithm.  The // exports comment
> only occurs in .6 files, not in the export data for .a files.
> This code is working because it is skipping past the real
> .a export data and pulling it out of the _go_.6 file inside
> the archive.  But that means the compiler and this code are
> looking at different data.  I will send a separate reply with
> the right code.
> 

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:166: return fmt.Sprintf("import error %s (offs =
%d): %s", e.pos, e.pos.Offset, e.err)
On 2011/04/07 16:27:19, rsc wrote:
> s/offs =/at byte offset/
> offs sounds plural

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:225: // ExportedName = ImportPath "." identifier
[ "·" [ int_lit ] ] .
On 2011/04/07 16:27:19, rsc wrote:
> The real syntax is
> 
> ExportedName = ImportPath "." identifier
> 
> where identifier is the Go definition plus · characters.
> It would be more robust to make the identifier scanner
> accept that definition.

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:286: // "[" already consumed
On 2011/04/07 16:27:19, rsc wrote:
> // "[" already consumed and lookahead known not to be "]"

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:290: n, err := strconv.Atoui64(lit)
On 2011/04/07 16:27:19, rsc wrote:
> Should this be using the Const routines instead?

Not sure. I think it depends on how often the length of an array is used later
in form of Const. Let's postpone this to later.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:313: name := "_"
On 2011/04/07 16:27:19, rsc wrote:
> name := ""
> 
> As written the code conflates ? and _, but they are not the same.
> 
> type T struct {
>     _ sync.Mutex
> }
> 
> type T struct {
>     sync.Mutex  // import metadata says "? sync.Mutex"
> }

Yes, I was aware of this (see respective TODO in parseField) - this was just a
work-around for now. Anyway, changed.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:431: // InterfaceType = "interface" "{" {
MethodSpec } "}" .
On 2011/04/07 16:27:19, rsc wrote:
> missing the ";" 

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:452: // for simplicity we accept: [ "<-" ] chan [
"<-" ] Type .
On 2011/04/07 16:27:19, rsc wrote:
> Doesn't this get the precedence wrong?
> See if you can import the result of compiling test/import2.go.
> 

Done. Don't know how many times I have to do this before I remember not to
shortcut.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:513: return &Slice{Ellipsis: true, Elt:
p.parseType()}
On 2011/04/07 16:27:19, rsc wrote:
> This is blurring syntax and semantics (see comment in other file).
> An alternative is to make the function parameter parser look for the ...

Not convinced. Inside the function the parameter type behaves exactly like a
slice. It's only when passing arguments that one needs to check, which is a
single special case.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:533: // If the ImportDecl were using the same
notation as all other
On 2011/04/07 16:27:19, rsc wrote:
> The exported name is a selector expression.
> An import path is not.  I don't understand why
> they would use the same syntax.  It's true that
> they are both an import path and a name,
> but they have different meanings.
> 
> Delete comment?

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:536: p.expect(scanner.Ident) // throw away for
now
On 2011/04/07 16:27:19, rsc wrote:
> // The identifier has no semantic meaning in the import data.
> // It exists so that error messages can print the real package
> // name: binary.ByteOrder instead of "encoding/binary".ByteOrder.
> // TODO(gri): Save import path -> package name mapping.
> p.expect(scanner.Ident)

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:565: return NewConst(token.FLOAT, mant+"e"+exp)
On 2011/04/07 16:27:19, rsc wrote:
> 3p5 means 3*2^5 = 96, but this rewrites it to 3e5 = 300000.

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:593: typ = Bool.Underlying
On 2011/04/07 16:27:19, rsc wrote:
> This is confusing.  Maybe there should be top-level names
> IdealBool, IdealComplex, IdealInt, IdealFloat, IdealString?

The underlying type of the predeclared types is in some ways their unnamed ideal
type. I like to push this a little further and see if I run into
inconsistencies.

> Also this should only set if typ == nil.

Yes, meant to do that.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:633: name.Underlying = Underlying(typ)
On 2011/04/07 16:27:19, rsc wrote:
> It should not be possible for typ to be a named type here,
> so the call to Underlying should be redundant.  Fine though.

Actually, this was wrong for basic types.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:670: // Decl = [ ImportDecl | ConstDecl| TypeDecl
| VarDecl | FuncDecl | MethodDecl ] "\n" .
On 2011/04/07 16:27:19, rsc wrote:
> s/ConstDecl/& /

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/testdata/ex...
File src/pkg/go/types/testdata/exports.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/testdata/ex...
src/pkg/go/types/testdata/exports.go:33: T6 chan<- int
On 2011/04/07 16:27:19, rsc wrote:
> Worth adding
>    chan (<-chan int)
>    chan<- (chan int)
> 
>    

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go
File src/pkg/go/types/types.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:13: // All types implement the Type interface.
On 2011/04/07 16:27:19, rsc wrote:
> Is the intent to expand this?  If so would the isType()
> method then become redundant?  

For now this is just here to ensure that we only store types into a Type.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:45: Ellipsis bool // true for "...Elt" types
On 2011/04/07 16:27:19, rsc wrote:
> The ... is a property of the function type, not a slice type.
> I think it's a mistake to stick it here, because any code
> that wants to deal with slices is going to have to look at it
> and think "hmm, what's going on with this?".  I suggest adding
> 
> IsVariadic bool
> 
> to the Func instead.

I am not convinced. ... types behave likes slices inside functions - there is no
difference. Only when passing to the function one has to special case. Seems
like that's one place compared to all the places where a ... type is used like a
slice.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:53: Fields *ast.Scope // always present
On 2011/04/07 16:27:19, rsc wrote:
> I'm surprised to see the ast mixed in here.
> I think it would be clearer to separate them
> completely, introducing a type Field and making
> this []*Field.  It makes sense for the type tree
> to have references back to the original ast, but
> this representation implies that the original
> ast then has references back to this package,
> because otherwise you couldn't find out the
> type of a field.  I think you'll have more flexibility
> if they're not so tightly coupled, the same way we
> have more flexibility when we avoid cyclic dependencies.
> 
> If this is just a placeholder for something else later,
> I'd suggest leaving it out for now to make sure it
> doesn't get depended on.
> 
> Same applies to the other uses of *ast.Scope here.

It's a placeholder. Removed for now.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:62: Base Type
On 2011/04/07 16:27:19, rsc wrote:
> can this be Elt?
> 
> Reflect uses the same name for all of these (Elem)
> and it would be nice if the mental translation was
> 1:1 (Elem -> Elt).
> 
> It would be even nicer if this package used the same
> speling as reflect (Ptr, Elem) but I'm not going to
> push for that.

It's really not an element. It's base type in the spec. The reflect names are
somewhat based on making the API small. Is it really important?

The AST uses Elt instead of Elem.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:69: Params *ast.Scope // always present
On 2011/04/07 16:27:19, rsc wrote:
> This is even more surprising as a scope, since
> the names involved are not relevant.  I expected
> something like
> 
> In []Type
> Out []Type
> IsVariadic bool

There's a TODO below. This was a placeholder. removed for now.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:78: Methods *ast.Scope // always present
On 2011/04/07 16:27:19, rsc wrote:
> Again.

Done.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:102: Scope      *ast.Scope  // associated methods (and
fields, if any); or nil
On 2011/04/07 16:27:19, rsc wrote:
> Again.

Done.

[rsc, 2011-04-08 03:25:43]

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go
File src/pkg/go/types/types.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:45: Ellipsis bool // true for "...Elt" types
On 2011/04/08 02:41:32, gri wrote:
> On 2011/04/07 16:27:19, rsc wrote:
> > The ... is a property of the function type, not a slice type.
> > I think it's a mistake to stick it here, because any code
> > that wants to deal with slices is going to have to look at it
> > and think "hmm, what's going on with this?".  I suggest adding
> > 
> > IsVariadic bool
> > 
> > to the Func instead.
> 
> I am not convinced. ... types behave likes slices inside functions - there is
no
> difference. Only when passing to the function one has to special case. 

Right.  These are arguments for making it a property of the
function and not distinguishing in the slice type itself.
That is, if you parse func(string, ...int), then I am
suggesting to construct something like:

FuncType{
    In: []Type{
        String,
        Slice{Elt: Int},
    },
    IsVariadic: true,
}

The result of parsing func(string, []int) would be identical,
except that it would have IsVariadic: false in the FuncType
(not in the type of the final input parameter).

The "property of the Slice" approach as written in the CL right now
is more suited to the way ... used to be defined, where you could write

func f(args ...int) {
    g(args)
}

func g(args ...int) {}

Then it really did matter that args had this magic "..." bit,
and you needed to know that bit when type-checking its use in the
call to g, so it made sense to be part of args's Slice type.
(This is what 6g used to do.)

But when we changed Go to make args ...int indistinguishable
inside the function from a []int, that "bit in the Slice type"
approach stopped making sense.  In every way, inside the body of f,
args has type []int, not ...int.  So its Slice should be the same
as the Slice for []int.

As one concrete example of how the current code would complicate
things, consider

func f(args ...int) {
    x := args
}

To type check the assignment, the natural thing to do is say
that x's Type = args's Type.  In the "Ellipsis inside Slice"
representation, this assignment is wrong.  The ellipsis bool
must be cleared, introducing a special case any time you 
copy the type of an expression.  (Or it introduces a special
case when computing the type of an expression that refers
to a parameter, if you want to push it down one level.
Somewhere, the Ellipsis bool has to get cleared during the
Slice's traveling from being the type of args to being the
type of x.)

The only time when the type checking algorithm needs to
know that f is variadic is while checking a call to f
or comparing two function types for equality.
Making IsVariadic a property of the FuncType handles both
these uses as well or better than making it a property of
the final argument type, and it avoids the special cases
such as the one involving assignment above.

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:62: Base Type
On 2011/04/08 02:41:32, gri wrote:
> On 2011/04/07 16:27:19, rsc wrote:
> > can this be Elt?
> > 
> > Reflect uses the same name for all of these (Elem)
> > and it would be nice if the mental translation was
> > 1:1 (Elem -> Elt).
> > 
> > It would be even nicer if this package used the same
> > speling as reflect (Ptr, Elem) but I'm not going to
> > push for that.
> 
> It's really not an element. It's base type in the spec. The reflect names are
> somewhat based on making the API small. Is it really important?

No, just a would be nice.  Not a big deal.

> The AST uses Elt instead of Elem.

Sure.

http://codereview.appspot.com/4314054/diff/30003/src/pkg/go/types/exportdata.go
File src/pkg/go/types/exportdata.go (right):

http://codereview.appspot.com/4314054/diff/30003/src/pkg/go/types/exportdata....
src/pkg/go/types/exportdata.go:42: file *os.File
change to embedded io.Closer
drop Close method

http://codereview.appspot.com/4314054/diff/30003/src/pkg/go/types/exportdata....
src/pkg/go/types/exportdata.go:64: err = fmt.Errorf("exportData(%s): %v",
filename, err)
exportData(%s) is somewhat cryptic for end users.

reading export data: %s:

?

[rsc, 2011-04-08 03:48:58]

LGTM

I meant to LGTM the last set of comments.
I strongly believe that IsVariadic belongs
in FuncType, not SliceType, but I won't press
the point beyond my explanation in the last mail.

[gri, 2011-04-08 04:08:59]

PTAL

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go
File src/pkg/go/types/types.go (right):

http://codereview.appspot.com/4314054/diff/27002/src/pkg/go/types/types.go#ne...
src/pkg/go/types/types.go:45: Ellipsis bool // true for "...Elt" types
On 2011/04/08 03:25:44, rsc wrote:
> On 2011/04/08 02:41:32, gri wrote:
> > On 2011/04/07 16:27:19, rsc wrote:
> > > The ... is a property of the function type, not a slice type.
> > > I think it's a mistake to stick it here, because any code
> > > that wants to deal with slices is going to have to look at it
> > > and think "hmm, what's going on with this?".  I suggest adding
> > > 
> > > IsVariadic bool
> > > 
> > > to the Func instead.
> > 
> > I am not convinced. ... types behave likes slices inside functions - there
is
> no
> > difference. Only when passing to the function one has to special case. 
> 
> Right.  These are arguments for making it a property of the
> function and not distinguishing in the slice type itself.
> That is, if you parse func(string, ...int), then I am
> suggesting to construct something like:
> 
> FuncType{
>     In: []Type{
>         String,
>         Slice{Elt: Int},
>     },
>     IsVariadic: true,
> }
> 
> The result of parsing func(string, []int) would be identical,
> except that it would have IsVariadic: false in the FuncType
> (not in the type of the final input parameter).
> 
> The "property of the Slice" approach as written in the CL right now
> is more suited to the way ... used to be defined, where you could write
> 
> func f(args ...int) {
>     g(args)
> }
> 
> func g(args ...int) {}
> 
> Then it really did matter that args had this magic "..." bit,
> and you needed to know that bit when type-checking its use in the
> call to g, so it made sense to be part of args's Slice type.
> (This is what 6g used to do.)
> 
> But when we changed Go to make args ...int indistinguishable
> inside the function from a []int, that "bit in the Slice type"
> approach stopped making sense.  In every way, inside the body of f,
> args has type []int, not ...int.  So its Slice should be the same
> as the Slice for []int.
> 
> As one concrete example of how the current code would complicate
> things, consider
> 
> func f(args ...int) {
>     x := args
> }
> 
> To type check the assignment, the natural thing to do is say
> that x's Type = args's Type.  In the "Ellipsis inside Slice"
> representation, this assignment is wrong.  The ellipsis bool
> must be cleared, introducing a special case any time you 
> copy the type of an expression.  (Or it introduces a special
> case when computing the type of an expression that refers
> to a parameter, if you want to push it down one level.
> Somewhere, the Ellipsis bool has to get cleared during the
> Slice's traveling from being the type of args to being the
> type of x.)
> 
> The only time when the type checking algorithm needs to
> know that f is variadic is while checking a call to f
> or comparing two function types for equality.
> Making IsVariadic a property of the FuncType handles both
> these uses as well or better than making it a property of
> the final argument type, and it avoids the special cases
> such as the one involving assignment above.
> 
ah yes. the assignment property makes it obvious.

http://codereview.appspot.com/4314054/diff/30003/src/pkg/go/types/exportdata.go
File src/pkg/go/types/exportdata.go (right):

http://codereview.appspot.com/4314054/diff/30003/src/pkg/go/types/exportdata....
src/pkg/go/types/exportdata.go:42: file *os.File
On 2011/04/08 03:25:44, rsc wrote:
> change to embedded io.Closer
> drop Close method

Done.

http://codereview.appspot.com/4314054/diff/30003/src/pkg/go/types/exportdata....
src/pkg/go/types/exportdata.go:64: err = fmt.Errorf("exportData(%s): %v",
filename, err)
On 2011/04/08 03:25:44, rsc wrote:
> exportData(%s) is somewhat cryptic for end users.
> 
> reading export data: %s:
> 
> ?

Done.

[rsc, 2011-04-08 04:18:02]

LGTM

Feel free to fix and submit.

http://codereview.appspot.com/4314054/diff/21003/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/4314054/diff/21003/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:399: assert(!*isVariadic)
Seems like f(...int, ...int) will trigger this.
if + real error message?

[gri, 2011-04-08 04:25:21]

On Thu, Apr 7, 2011 at 9:18 PM,  <rsc@golang.org> wrote:
> LGTM
>
> Feel free to fix and submit.
>
>
>
>
http://codereview.appspot.com/4314054/diff/21003/src/pkg/go/types/gcimporter.go
> File src/pkg/go/types/gcimporter.go (right):
>
>
http://codereview.appspot.com/4314054/diff/21003/src/pkg/go/types/gcimporter....
> src/pkg/go/types/gcimporter.go:399: assert(!*isVariadic)
> Seems like f(...int, ...int) will trigger this.
> if + real error message?

yes, but should never happen. anyway, give real error message now.

As an aside: The parsing of ... (here and in go/parser) would actually
be slightly simpler if the syntax were func f(x int...) (we had this
discussion before). Would be more in line with f(x...). I am always
thinking twice when I write ... (before or after?).

- Robert


>
> http://codereview.appspot.com/4314054/
>

[rsc, 2011-04-08 04:35:56]

> As an aside: The parsing of ... (here and in go/parser) would actually
> be slightly simpler if the syntax were func f(x int...) (we had this
> discussion before). Would be more in line with f(x...). I am always
> thinking twice when I write ... (before or after?).

The way I remember is that all our type modifiers are prefix operators.

Russ

[gri, 2011-04-08 04:39:29]

On Thu, Apr 7, 2011 at 9:35 PM, Russ Cox <rsc@golang.org> wrote:
>> As an aside: The parsing of ... (here and in go/parser) would actually
>> be slightly simpler if the syntax were func f(x int...) (we had this
>> discussion before). Would be more in line with f(x...). I am always
>> thinking twice when I write ... (before or after?).
>
> The way I remember is that all our type modifiers are prefix operators.

except that it is not really a type modifier but a function signature
modifier (supported by our previous discussion about the IsVariadic
information)... :-)

>
> Russ
>

[gri, 2011-04-08 04:40:36]

Hello eds, rog, rsc (cc: golang-dev@googlegroups.com),

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

[gri, 2011-04-08 04:40:45]

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

go/types: New Go type hierarchy implementation for AST.

This CL defines a new, more Go-like representation of
Go types (different structs for different types as
opposed to a single Type node). It also implements
an ast.Importer for object/archive files generated
by the gc compiler tool chain. Besides the individual
type structs, the main difference is the handling of
named types: In the old world, a named type had a
non-nil *Object pointer but otherwise looked no
different from other types. In this new model, named
types have their own representation types.Name. As
a result, resolving cycles is a bit simpler during
construction, at the cost of having to deal with
types.Name nodes explicitly later. It remains to be
seen if this is a good approach. Nevertheless, code
involving types reads more nicely and benefits from
full type checking. Also, the representation seems
to more closely match the spec wording.

Credits: The original version of the gc importer was
written by Evan Shaw (chickencha@gmail.com). The new
version in this CL is based largely on Evan's original
code but contains bug fixes, a few simplifications,
some restructuring, and was adjusted to use the
new type hierarchy. I have added a comprehensive test
that imports all packages found under $GOROOT/pkg (with
a 3s time-out to limit the run-time of the test). Run
gotest -v for details.

The original version of ExportData (exportdata.go) was
written by Russ Cox (rsc@golang.org). The current version
is returning the internal buffer positioned at the beginning
of the export data instead of printing the export data to
stdout.

With the new types package, the existing in-progress
typechecker package is deprecated. I will delete it
once all functionality has been brought over.

R=eds, rog, rsc
CC=golang-dev
http://codereview.appspot.com/4314054

[rsc, 2011-04-08 04:41:27]

LGTM

[gri, 2011-04-08 04:41:59]

I don't know why hg sent this. Please ignore.
- gri

On Thu, Apr 7, 2011 at 9:40 PM,  <gri@golang.org> wrote:
> Hello eds, rog, rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> http://codereview.appspot.com/4314054/
>

[rsc, 2011-04-08 04:43:20]

On Fri, Apr 8, 2011 at 00:41, Robert Griesemer <gri@golang.org> wrote:
> I don't know why hg sent this. Please ignore.
> - gri

I do.  That can be the next codereview bug I fix.
The "Mailed:" bit is not propagated during clpatch.

Russ