FAQ
LGTM



http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go
File src/pkg/exp/types/staging/types.go (right):

http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode30
src/pkg/exp/types/staging/types.go:30: // 3) If a non-nil but non-empty
types map is provided, Check type-
I have minor discomfort about treating len 0 map separately from len > 0
map. It means if you have a list of things you care about and you do
something like

m := make(map[ast.Expr]Type)
for _, thing := range things {
m[thing] = nil
}

then you get fast behavior except when len(things) == 0. It might be
worth making this a separate function. CheckSubset or something like
that.
Like I said, only minor discomfort.

http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode42
src/pkg/exp/types/staging/types.go:42: // All types implement the Type
interface.
Is there anything else callers should know about types? Can they be
compared with == ?
Are there other common methods they'll have?

I wonder if all Types should have a few things like Kind() Kind
(replacing BasicKind with a few more things like Struct Func etc) and
Info() Info. Just a suggestion, though. I'm sure you've weighed these
before.

http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode127
src/pkg/exp/types/staging/types.go:127: Typ Type
s/Typ/Type/?

http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode129
src/pkg/exp/types/staging/types.go:129: IsAnonymous bool
Make comments clear about whether Name == "" for anonymous.

http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode150
src/pkg/exp/types/staging/types.go:150: // A Signature represents a
user-defined function type func(...) (...).
Not a Func?

http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode151
src/pkg/exp/types/staging/types.go:151: // TODO(gri) consider using
tuples to represent parameters and results.
Please don't expose 'tuples'. So many people are confused about whether
Go has tuples already. If the concept must be exposed it would be better
as a different name, like 'List' or 'ReturnList'. But probably ObjList
is okay.

http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/universe.go
File src/pkg/exp/types/staging/universe.go (right):

http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/universe.go#newcode21
src/pkg/exp/types/staging/universe.go:21: var Typ = [...]*Basic{
s/Typ/BasicType/ ?
Or maybe BasicKind should have a Type() Type method that indexes this
(then unexported) table? Bool.Type() etc.

http://codereview.appspot.com/6490089/

Search Discussions

  • Roger peppe at Sep 10, 2012 at 6:53 pm

    On 10 September 2012 18:37, wrote:
    LGTM



    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go
    File src/pkg/exp/types/staging/types.go (right):

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode30
    src/pkg/exp/types/staging/types.go:30: // 3) If a non-nil but non-empty
    types map is provided, Check type-
    I have minor discomfort about treating len 0 map separately from len > 0
    map. It means if you have a list of things you care about and you do
    something like

    m := make(map[ast.Expr]Type)
    for _, thing := range things {
    m[thing] = nil
    }

    then you get fast behavior except when len(things) == 0. It might be
    worth making this a separate function. CheckSubset or something like
    that.
    Like I said, only minor discomfort.

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode42
    src/pkg/exp/types/staging/types.go:42: // All types implement the Type
    interface.
    Is there anything else callers should know about types? Can they be
    compared with == ?
    Are there other common methods they'll have?

    I wonder if all Types should have a few things like Kind() Kind
    (replacing BasicKind with a few more things like Struct Func etc) and
    Info() Info. Just a suggestion, though. I'm sure you've weighed these
    before.
    It occurs to me that people might find this more approachable
    if at least some of the API looked similar to the reflect Type API.
    That is, each concrete type (still exported) could implement an interface
    that looked pretty much the same as reflect.Type. It could perhaps
    even use the same constants for Kind.

    Just a thought.
  • Robert Griesemer at Sep 10, 2012 at 9:53 pm
    I think if that is a desirable goal it should be fairly easy to change (or
    add) later as it's essentially a thin veneer on top of what's here now.

    I've tried an approach where every type is simply a special kind of
    concrete Type (no hierarchy), and while that may have some (performance)
    advantages it really doesn't use the Go type system very well but instead
    relies on run-time panics in case of errors. The current reflection
    implementation is the result of several iterations, and one of the driving
    concerns there was performance and minimum amount of allocation. Arguably
    it would be nicer if each type had its own corresponding Go type (as was
    the case in the beginning).

    I've been iterating this (too) many times and each approach has pros and
    cons. At this point my goal is to get this fully working. Once we're there
    we can fine-tune. I agree that having both APIs somewhat similar might be
    nice.

    - gri


    On Mon, Sep 10, 2012 at 11:47 AM, roger peppe wrote:
    On 10 September 2012 18:37, wrote:
    LGTM



    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go
    File src/pkg/exp/types/staging/types.go (right):

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode30
    src/pkg/exp/types/staging/types.go:30: // 3) If a non-nil but non-empty
    types map is provided, Check type-
    I have minor discomfort about treating len 0 map separately from len > 0
    map. It means if you have a list of things you care about and you do
    something like

    m := make(map[ast.Expr]Type)
    for _, thing := range things {
    m[thing] = nil
    }

    then you get fast behavior except when len(things) == 0. It might be
    worth making this a separate function. CheckSubset or something like
    that.
    Like I said, only minor discomfort.

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode42
    src/pkg/exp/types/staging/types.go:42: // All types implement the Type
    interface.
    Is there anything else callers should know about types? Can they be
    compared with == ?
    Are there other common methods they'll have?

    I wonder if all Types should have a few things like Kind() Kind
    (replacing BasicKind with a few more things like Struct Func etc) and
    Info() Info. Just a suggestion, though. I'm sure you've weighed these
    before.
    It occurs to me that people might find this more approachable
    if at least some of the API looked similar to the reflect Type API.
    That is, each concrete type (still exported) could implement an interface
    that looked pretty much the same as reflect.Type. It could perhaps
    even use the same constants for Kind.

    Just a thought.
  • Gri at Sep 10, 2012 at 9:39 pm
    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go
    File src/pkg/exp/types/staging/types.go (right):

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode30
    src/pkg/exp/types/staging/types.go:30: // 3) If a non-nil but non-empty
    types map is provided, Check type-
    On 2012/09/10 17:37:18, rsc wrote:
    I have minor discomfort about treating len 0 map separately from len > 0 map. It
    means if you have a list of things you care about and you do something like
    m := make(map[ast.Expr]Type)
    for _, thing := range things {
    m[thing] = nil
    }
    then you get fast behavior except when len(things) == 0. It might be worth
    making this a separate function. CheckSubset or something like that.
    Like I said, only minor discomfort.

    It's a valid concern. CheckSubset/checkPartial was just an idea that I
    thought would be quite useful for things like go fix where you only need
    some of the information. Leaving it away for now, easy to add, hard to
    remove once it's here.

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode42
    src/pkg/exp/types/staging/types.go:42: // All types implement the Type
    interface.
    On 2012/09/10 17:37:19, rsc wrote:
    Is there anything else callers should know about types? Can they be compared
    with == ?
    Are there other common methods they'll have?
    I wonder if all Types should have a few things like Kind() Kind
    (replacing
    BasicKind with a few more things like Struct Func etc) and Info()
    Info. Just a
    suggestion, though. I'm sure you've weighed these before.
    Probably there should be more. For instance there are predicates
    isIdentical, isBoolean, isString, etc. that could be exported and/or
    made methods of Type. For now I kept it at the minimum because it's easy
    to add those things once it all works (but tedious to maintain before).

    Added comment.

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode127
    src/pkg/exp/types/staging/types.go:127: Typ Type
    On 2012/09/10 17:37:19, rsc wrote:
    s/Typ/Type/?
    I'me using typ for all the non-exported fields, hence Typ in this case.
    Changed.

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode129
    src/pkg/exp/types/staging/types.go:129: IsAnonymous bool
    On 2012/09/10 17:37:19, rsc wrote:
    Make comments clear about whether Name == "" for anonymous.
    Done.

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode150
    src/pkg/exp/types/staging/types.go:150: // A Signature represents a
    user-defined function type func(...) (...).
    On 2012/09/10 17:37:19, rsc wrote:
    Not a Func?
    I'm considering having a much better type Objects hierarchy, where we'd
    have Consts, Vars, Funcs, etc. and then I cannot reuse Func here.
    Signature seemed pretty clear. Happy to reconsider if things turn out
    differently.

    http://codereview.appspot.com/6490089/diff/1018/src/pkg/exp/types/staging/types.go#newcode151
    src/pkg/exp/types/staging/types.go:151: // TODO(gri) consider using
    tuples to represent parameters and results.
    On 2012/09/10 17:37:19, rsc wrote:
    Please don't expose 'tuples'. So many people are confused about
    whether Go has
    tuples already. If the concept must be exposed it would be better as a different
    name, like 'List' or 'ReturnList'. But probably ObjList is okay.


    ACK. Will change. Added comment for now.

    http://codereview.appspot.com/6490089/
  • Gri at Sep 10, 2012 at 9:54 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=26b1cb07dc52 ***

    exp/types/staging: typechecker API

    First set of type checker files for review.
    The primary concern here is the typechecker
    API (types.go).

    R=rsc, adonovan, r, rogpeppe
    CC=golang-dev
    http://codereview.appspot.com/6490089


    http://codereview.appspot.com/6490089/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 10, '12 at 5:37p
activeSep 10, '12 at 9:54p
posts5
users3
websitegolang.org

3 users in discussion

Gri: 3 posts Rsc: 1 post Roger peppe: 1 post

People

Translate

site design / logo © 2021 Grokbase