-
Notifications
You must be signed in to change notification settings - Fork 208
add multiverse lookup to Type #309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ package types | |
| import ( | ||
| gotypes "go/types" | ||
| "strings" | ||
| "sync" | ||
| ) | ||
|
|
||
| // Ref makes a reference to the given type. It can only be used for e.g. | ||
|
|
@@ -364,6 +365,50 @@ type Type struct { | |
|
|
||
| // The underlying Go type. | ||
| GoType gotypes.Type | ||
|
|
||
| // The reference to Multiverse | ||
| multiverse *multiverse | ||
| } | ||
|
|
||
| // multiverse holds Type definitions that were not found in the imported code but are needed during | ||
| // generation. For example the imported code my have T but not *T, while the generated code needs *T. This | ||
| // can't be part of the main Universe because that is a simple map, with no locking, and we all know what happens | ||
| // when you modify a map while it is being iterated. Storing the multiverse alongside a Type ensures that there's | ||
| // at most one *Type for every type, and maintains the invariant that PointerTo(String) == PointerTo(String). | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Super not since I am leaving other comments. s/String/T in the invariant |
||
| type multiverse struct { | ||
| real Universe | ||
| mu sync.Mutex | ||
| synthetic map[string]*Type | ||
| } | ||
|
|
||
| // InitMultiverse inits a multiverse for a Type. | ||
| // It panics if called twice. | ||
| func (t *Type) InitMultiverse(u Universe) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just call it Init -- multiverse is an implementation detail, it doesn't need to leak into the API |
||
| if t.multiverse != nil { | ||
| panic("Can't initialize a non-empty multiverse on Type") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "can't re-initialize type %v"... |
||
| } | ||
| t.multiverse = &multiverse{ | ||
| real: u, | ||
| mu: sync.Mutex{}, | ||
| synthetic: map[string]*Type{}, | ||
| } | ||
| } | ||
|
|
||
| // GetOrAddType searches a Type in the Universe and synthetic map. | ||
| // If there is a matching name, return the Type, otherwise, create the Type. | ||
| func (m *multiverse) GetOrAddType(t *Type) *Type { | ||
| if p, ok := m.real[t.Name.Package]; ok { | ||
ziyue-101 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if t, ok := p.Types[t.Name.Name]; ok { | ||
| return t | ||
| } | ||
| } | ||
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
| if t, ok := m.synthetic[t.Name.Name]; ok { | ||
| return t | ||
| } | ||
| m.synthetic[t.Name.Name] = t | ||
| return t | ||
| } | ||
|
|
||
| // String returns the name of the type. | ||
|
|
@@ -556,13 +601,14 @@ var ( | |
| ) | ||
|
|
||
| func PointerTo(t *Type) *Type { | ||
| return &Type{ | ||
| pt := &Type{ | ||
| Name: Name{ | ||
| Name: "*" + t.Name.String(), | ||
| }, | ||
| Kind: Pointer, | ||
| Elem: t, | ||
| } | ||
| return t.multiverse.GetOrAddType(pt) | ||
| } | ||
|
|
||
| func IsInteger(t *Type) bool { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ limitations under the License. | |
| package types | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "sync" | ||
| "testing" | ||
| ) | ||
|
|
||
|
|
@@ -37,6 +39,95 @@ func TestGetBuiltin(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestPointerTo(t *testing.T) { | ||
| type1 := &Type{ | ||
| Name: Name{Package: "pkgname", Name: "structname"}, | ||
| Kind: Struct, | ||
| } | ||
| type2 := &Type{ | ||
| Name: Name{Package: "pkgname", Name: "secondstructname"}, | ||
| Kind: Struct, | ||
| } | ||
|
|
||
| u := Universe{ | ||
| "pkgname": &Package{ | ||
| Types: map[string]*Type{ | ||
| "structname": type1, | ||
| "secondstructname": type2, | ||
| }, | ||
| }, | ||
| "": &Package{ | ||
| Types: map[string]*Type{ | ||
| "*pkgname.structname": &Type{ | ||
| Name: Name{Name: "*pkgname.structname"}, | ||
| Kind: Pointer, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| type3 := &Type{ | ||
| Name: Name{Package: "pkgname", Name: "thridstructname"}, | ||
| Kind: Struct, | ||
| multiverse: &multiverse{ | ||
| real: u, | ||
| synthetic: map[string]*Type{ | ||
| "*pkgname.thridstructname": &Type{ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/thrid/third Or just call these T1, T2, T3 :) |
||
| Name: Name{Name: "*pkgname.thridstructname"}, | ||
| Kind: Pointer, | ||
| }, | ||
| }, | ||
| mu: sync.Mutex{}, | ||
| }, | ||
| } | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| tp *Type | ||
| expected *Type | ||
| expectCreation bool | ||
| }{ | ||
| { | ||
| name: "universe has the pointer type", | ||
| tp: type1, | ||
| expected: u[""].Types["*pkgname.structname"], | ||
| expectCreation: false, | ||
| }, | ||
| { | ||
| name: "neither universe or cache has the pointer type", | ||
| tp: type2, | ||
| expected: &Type{ | ||
| Name: Name{Name: "*pkgname.secondstructname"}, | ||
| Kind: Pointer, | ||
| Elem: type2, | ||
| }, | ||
| expectCreation: true, | ||
| }, | ||
| { | ||
| name: "cache has the pointer type", | ||
| tp: type3, | ||
| expected: type3.multiverse.synthetic["*pkgname.thridstructname"], | ||
| expectCreation: false, | ||
| }, | ||
| } | ||
| for _, tc := range testCases { | ||
| if tc.tp.multiverse == nil { | ||
| tc.tp.multiverse = &multiverse{ | ||
| real: u, | ||
| synthetic: map[string]*Type{}, | ||
| mu: sync.Mutex{}, | ||
| } | ||
| } | ||
| tp := PointerTo(tc.tp) | ||
| if tc.expectCreation && !reflect.DeepEqual(tp, tc.expected) { | ||
| t.Errorf("PointerTo failed, expected %v, got : %v", tc.expected, tp) | ||
| } | ||
| if !tc.expectCreation && tp != tc.expected { | ||
| t.Errorf("PointerTo should not create a new pointer type, expected %v, got : %v", tc.expected, tp) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestGetMarker(t *testing.T) { | ||
| u := Universe{} | ||
| n := Name{Package: "path/to/package", Name: "Foo"} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized someone needs to call this on all of the builtin types. This means that there can be only 1 Universe because the builtins are global variables, which have a single multiverse pointer each. That makes me sad.
Can you think of any alternative that is not so limited?
Is the cure worse than the sickness? I don't like that PointerTo(T) != PointerTo(T) but if there were two universes active T != T, anyway...
Have to think on this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisiting this - I still don't know what we should do.
Large-scale redesign of gengo apps (e.g. making the Universe the central abstraction, passing it everywhere, so instead of
types.Stringyou might useu.String()oru.PointerTo(t)) seems unlikely.Making a global universe (so there can't be more than one) seems plausible but amounts to gengo v3, and I don't see anyone working on that right now, especially without some other driver.
We could pre-populate a
t.Pointerup to some level of depth (e.g. ***T) and thast "should be enough for anyone" but requires more gengo work.I'm letting this sit for now until it is clearer what we need.