From 216dabac643328d2c01fd52463d0cc22d4874776 Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Fri, 30 Oct 2020 09:39:33 +0100 Subject: [PATCH 1/7] codegen: create speciated Lookup method on maps. No surprise this is needed; just filling in the gaps. --- schema/gen/go/genMap.go | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/schema/gen/go/genMap.go b/schema/gen/go/genMap.go index ccb91dfe..45b94638 100644 --- a/schema/gen/go/genMap.go +++ b/schema/gen/go/genMap.go @@ -45,18 +45,31 @@ func (g mapGenerator) EmitNativeType(w io.Writer) { func (g mapGenerator) EmitNativeAccessors(w io.Writer) { // Generate a speciated Lookup as well as LookupMaybe method. - // The LookupMaybe method is needed if the map value is nullable and you're going to distinguish nulls - // (and may also be convenient if you would rather handle Maybe_Absent than an error for not-found). - // The Lookup method works fine if the map value isn't nullable - // (and should be preferred in that case, because boxing something into a maybe when it wasn't already stored that way costs an alloc(!), - // and may additionally incur a memcpy if the maybe for the value type doesn't use pointers internally). - // REVIEW: is there a way we can make this less twisty? it is VERY unfortunate if the user has to know what sort of map it is to know which method to prefer. - // Maybe the Lookup method on maps that have nullable values should just always have a MaybeT return type? - // But then this means the Lookup method doesn't "need" an error as part of its return signiture, which just shuffles differences around. + // The Lookup method returns nil in case of *either* an absent value or a null value, + // and so should only be used if the map type doesn't allow nullable keys or if the caller doesn't care about the difference. + // The LookupMaybe method returns a MaybeT type for the map value, + // and is needed if the map allows nullable values and the caller wishes to distinguish between null and absent. + // (The Lookup method should be preferred for maps that have non-nullable keys, because LookupMaybe may incur additional costs; + // boxing something into a maybe when it wasn't already stored that way costs an alloc(!), + // and may additionally incur a memcpy if the maybe for the value type doesn't use pointers internally). doTemplate(` + func (n *_{{ .Type | TypeSymbol }}) Lookup(k {{ .Type.KeyType | TypeSymbol }}) {{ .Type.ValueType | TypeSymbol }} { + v, exists := n.m[*k] + if !exists { + return nil + } + {{- if .Type.ValueIsNullable }} + if v.m == schema.Maybe_Null { + return nil + } + return {{ if not (MaybeUsesPtr .Type.ValueType) }}&{{end}}v.v + {{- else}} + return v + {{- end}} + } func (n *_{{ .Type | TypeSymbol }}) LookupMaybe(k {{ .Type.KeyType | TypeSymbol }}) Maybe{{ .Type.ValueType | TypeSymbol }} { - v, ok := n.m[*k] - if !ok { + v, exists := n.m[*k] + if !exists { return &_{{ .Type | TypeSymbol }}__valueAbsent } {{- if .Type.ValueIsNullable }} @@ -70,8 +83,6 @@ func (g mapGenerator) EmitNativeAccessors(w io.Writer) { } var _{{ .Type | TypeSymbol }}__valueAbsent = _{{ .Type.ValueType | TypeSymbol }}__Maybe{m:schema.Maybe_Absent} - - // TODO generate also a plain Lookup method that doesn't box and alloc if this type contains non-nullable values! `, w, g.AdjCfg, g) // FUTURE: also a speciated iterator? } From cf72a055b7085949a25434be580a8c32a8d0bbe4 Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Fri, 30 Oct 2020 10:22:31 +0100 Subject: [PATCH 2/7] codegen: create speciated iterator on maps. No suprise this is needed; just filling in the gaps. --- schema/gen/go/genMap.go | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/schema/gen/go/genMap.go b/schema/gen/go/genMap.go index 45b94638..8b36d67e 100644 --- a/schema/gen/go/genMap.go +++ b/schema/gen/go/genMap.go @@ -84,7 +84,39 @@ func (g mapGenerator) EmitNativeAccessors(w io.Writer) { var _{{ .Type | TypeSymbol }}__valueAbsent = _{{ .Type.ValueType | TypeSymbol }}__Maybe{m:schema.Maybe_Absent} `, w, g.AdjCfg, g) - // FUTURE: also a speciated iterator? + + // Generate a speciated iterator. + // The main advantage of this over the general ipld.MapIterator is of course keeping types visible (and concrete, to the compiler's eyes in optimizations, too). + // It also elides the error return from the iterator's Next method. (Overreads will result in nil keys; this is both easily avoidable, and unambiguous if you do goof and hit it.) + doTemplate(` + func (n {{ .Type | TypeSymbol }}) Iterator() *{{ .Type | TypeSymbol }}__Itr { + return &{{ .Type | TypeSymbol }}__Itr{n, 0} + } + + type {{ .Type | TypeSymbol }}__Itr struct { + n {{ .Type | TypeSymbol }} + idx int + } + + func (itr *{{ .Type | TypeSymbol }}__Itr) Next() (k {{ .Type.KeyType | TypeSymbol }}, v {{if .Type.ValueIsNullable }}Maybe{{end}}{{ .Type.ValueType | TypeSymbol }}) { + if itr.idx >= len(itr.n.t) { + return nil, nil + } + x := &itr.n.t[itr.idx] + k = &x.k + {{- if .Type.ValueIsNullable }} + v = &x.v + {{- else}} + v = &x.v + {{- end}} + itr.idx++ + return + } + func (itr *{{ .Type | TypeSymbol }}__Itr) Done() bool { + return itr.idx >= len(itr.n.t) + } + + `, w, g.AdjCfg, g) } func (g mapGenerator) EmitNativeBuilder(w io.Writer) { From a9982d521e1c21e36c078757e335ab6951c7d7db Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Fri, 30 Oct 2020 10:23:07 +0100 Subject: [PATCH 3/7] codegen: whitespace typo fix. --- schema/gen/go/genStruct.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/schema/gen/go/genStruct.go b/schema/gen/go/genStruct.go index 0e1e8153..a49e561f 100644 --- a/schema/gen/go/genStruct.go +++ b/schema/gen/go/genStruct.go @@ -33,7 +33,7 @@ func (g structGenerator) EmitNativeAccessors(w io.Writer) { doTemplate(` {{- $type := .Type -}} {{- /* ranging modifies dot, unhelpfully */ -}} {{- range $field := .Type.Fields }} - func (n _{{ $type | TypeSymbol }}) Field{{ $field | FieldSymbolUpper }}() {{ if $field.IsMaybe }}Maybe{{end}}{{ $field.Type | TypeSymbol }} { + func (n _{{ $type | TypeSymbol }}) Field{{ $field | FieldSymbolUpper }}() {{ if $field.IsMaybe }}Maybe{{end}}{{ $field.Type | TypeSymbol }} { return &n.{{ $field | FieldSymbolLower }} } {{- end}} From ea354c478ce00feb02132c61011dab43e0b8ed15 Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Fri, 30 Oct 2020 10:32:15 +0100 Subject: [PATCH 4/7] schema-schema: downcase middles of compound nouns. The capitalization on this has varied a bit over time. It's been tempting to capitalize these things because they're clearly two english words. However, I'm taking the line that they're a single word that just happens to have been derived from two english words, and such a neologism does not retain mid-word capitalization. (I'm looking at this right now because I'm attempting to write some new code around the schema-schema outputs, and so I want any dissonance and inconsistency gone from the start in this new code.) --- .../go/_demo/omg-schemas/schemaschema_test.go | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/schema/gen/go/_demo/omg-schemas/schemaschema_test.go b/schema/gen/go/_demo/omg-schemas/schemaschema_test.go index 057ac99f..6903f03e 100644 --- a/schema/gen/go/_demo/omg-schemas/schemaschema_test.go +++ b/schema/gen/go/_demo/omg-schemas/schemaschema_test.go @@ -132,27 +132,27 @@ func init() { ts.Accumulate(schema.SpawnUnion("MapRepresentation", []schema.TypeName{ "MapRepresentation_Map", - "MapRepresentation_StringPairs", - "MapRepresentation_ListPairs", + "MapRepresentation_Stringpairs", + "MapRepresentation_Listpairs", }, schema.SpawnUnionRepresentationKeyed(map[string]schema.TypeName{ "map": "MapRepresentation_Map", - "stringpairs": "MapRepresentation_StringPairs", - "listpairs": "MapRepresentation_ListPairs", + "stringpairs": "MapRepresentation_Stringpairs", + "listpairs": "MapRepresentation_Listpairs", }), )) ts.Accumulate(schema.SpawnStruct("MapRepresentation_Map", []schema.StructField{}, schema.StructRepresentation_Map{}, )) - ts.Accumulate(schema.SpawnStruct("MapRepresentation_StringPairs", + ts.Accumulate(schema.SpawnStruct("MapRepresentation_Stringpairs", []schema.StructField{ schema.SpawnStructField("innerDelim", "String", false, false), schema.SpawnStructField("entryDelim", "String", false, false), }, schema.StructRepresentation_Map{}, )) - ts.Accumulate(schema.SpawnStruct("MapRepresentation_ListPairs", + ts.Accumulate(schema.SpawnStruct("MapRepresentation_Listpairs", []schema.StructField{}, schema.StructRepresentation_Map{}, )) @@ -260,16 +260,16 @@ func init() { []schema.TypeName{ "StructRepresentation_Map", "StructRepresentation_Tuple", - "StructRepresentation_StringPairs", - "StructRepresentation_StringJoin", - "StructRepresentation_ListPairs", + "StructRepresentation_Stringpairs", + "StructRepresentation_Stringjoin", + "StructRepresentation_Listpairs", }, schema.SpawnUnionRepresentationKeyed(map[string]schema.TypeName{ "map": "StructRepresentation_Map", "tuple": "StructRepresentation_Tuple", - "stringpairs": "StructRepresentation_StringPairs", - "stringjoin": "StructRepresentation_StringJoin", - "listpairs": "StructRepresentation_ListPairs", + "stringpairs": "StructRepresentation_Stringpairs", + "stringjoin": "StructRepresentation_Stringjoin", + "listpairs": "StructRepresentation_Listpairs", }), )) ts.Accumulate(schema.SpawnStruct("StructRepresentation_Map", @@ -297,21 +297,21 @@ func init() { ts.Accumulate(schema.SpawnList("List__FieldName", "FieldName", false, )) - ts.Accumulate(schema.SpawnStruct("StructRepresentation_StringPairs", + ts.Accumulate(schema.SpawnStruct("StructRepresentation_Stringpairs", []schema.StructField{ schema.SpawnStructField("innerDelim", "String", false, false), schema.SpawnStructField("entryDelim", "String", false, false), }, schema.StructRepresentation_Map{}, )) - ts.Accumulate(schema.SpawnStruct("StructRepresentation_StringJoin", + ts.Accumulate(schema.SpawnStruct("StructRepresentation_Stringjoin", []schema.StructField{ schema.SpawnStructField("join", "String", false, false), // review: "delim" would seem more consistent with others -- but this is currently what the schema-schema says. schema.SpawnStructField("fieldOrder", "List__FieldName", true, false), // todo: dodging inline defn's again. }, schema.StructRepresentation_Map{}, )) - ts.Accumulate(schema.SpawnStruct("StructRepresentation_ListPairs", + ts.Accumulate(schema.SpawnStruct("StructRepresentation_Listpairs", []schema.StructField{}, schema.StructRepresentation_Map{}, )) From d5b6cd3a9fa1864bebfdfd10c876a5ac377b9114 Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Fri, 30 Oct 2020 11:27:25 +0100 Subject: [PATCH 5/7] schema-schema: add "stringprefix" to union strategies. Absence of this is an oversight, and I just happened to catch it while passing through the vicinity. Also: dropped a comment for later review on the bytesprefix strategy. While adding the stringprefix strategy, it's hard not to notice that variable length strings are allowed; so, it occurs to me we should probably do the same for byteprefix. (Also, possibly renaming it to byte*s*prefix.) Doing this would also fix the ancient weirdness of the map being flipped in an awkward way to evade int keys, which is a very happy coincidence (and in retrospect, I'm not sure why we didn't think of this solution earlier). --- .../go/_demo/omg-schemas/schemaschema_test.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/schema/gen/go/_demo/omg-schemas/schemaschema_test.go b/schema/gen/go/_demo/omg-schemas/schemaschema_test.go index 6903f03e..e8f6b42c 100644 --- a/schema/gen/go/_demo/omg-schemas/schemaschema_test.go +++ b/schema/gen/go/_demo/omg-schemas/schemaschema_test.go @@ -193,14 +193,16 @@ func init() { "UnionRepresentation_Keyed", "UnionRepresentation_Envelope", "UnionRepresentation_Inline", + "UnionRepresentation_StringPrefix", "UnionRepresentation_BytePrefix", }, schema.SpawnUnionRepresentationKeyed(map[string]schema.TypeName{ - "kinded": "UnionRepresentation_Kinded", - "keyed": "UnionRepresentation_Keyed", - "envelope": "UnionRepresentation_Envelope", - "inline": "UnionRepresentation_Inline", - "byteprefix": "UnionRepresentation_BytePrefix", + "kinded": "UnionRepresentation_Kinded", + "keyed": "UnionRepresentation_Keyed", + "envelope": "UnionRepresentation_Envelope", + "inline": "UnionRepresentation_Inline", + "stringprefix": "UnionRepresentation_StringPrefix", + "byteprefix": "UnionRepresentation_BytePrefix", }), )) ts.Accumulate(schema.SpawnMap("UnionRepresentation_Kinded", @@ -224,8 +226,15 @@ func init() { }, schema.StructRepresentation_Map{}, )) + ts.Accumulate(schema.SpawnStruct("UnionRepresentation_StringPrefix", + []schema.StructField{ + schema.SpawnStructField("discriminantTable", "Map__String__TypeName", false, false), // todo: dodging inline defn's again. + }, + schema.StructRepresentation_Map{}, + )) ts.Accumulate(schema.SpawnStruct("UnionRepresentation_BytePrefix", []schema.StructField{ + // REVIEW: for schema-schema overall: this is a very funny type. Should we use strings here? And perhaps make it use hex for maximum clarity? This would also allow multi-byte prefixes, which would match what's already done by stringprefix representation. schema.SpawnStructField("discriminantTable", "Map__TypeName__Int", false, false), // todo: dodging inline defn's again. }, schema.StructRepresentation_Map{}, From 95501bb0834455fdf67507e889fe9b6b3d027b3b Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Fri, 30 Oct 2020 15:22:00 +0100 Subject: [PATCH 6/7] codegen: simplify speciated iterator for maps. --- schema/gen/go/genMap.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/schema/gen/go/genMap.go b/schema/gen/go/genMap.go index 8b36d67e..ba31f0aa 100644 --- a/schema/gen/go/genMap.go +++ b/schema/gen/go/genMap.go @@ -104,11 +104,7 @@ func (g mapGenerator) EmitNativeAccessors(w io.Writer) { } x := &itr.n.t[itr.idx] k = &x.k - {{- if .Type.ValueIsNullable }} - v = &x.v - {{- else}} v = &x.v - {{- end}} itr.idx++ return } From cc2af4a28088636cd8cab8906d870f4cea3b8806 Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Fri, 30 Oct 2020 15:22:19 +0100 Subject: [PATCH 7/7] codegen: speciated lookups and iterators for typed lists. --- schema/gen/go/genList.go | 69 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/schema/gen/go/genList.go b/schema/gen/go/genList.go index cfe34bdc..32cbcb28 100644 --- a/schema/gen/go/genList.go +++ b/schema/gen/go/genList.go @@ -29,7 +29,74 @@ func (g listGenerator) EmitNativeType(w io.Writer) { } func (g listGenerator) EmitNativeAccessors(w io.Writer) { - // FUTURE: come back to this -- surely something nice can be done here. + // Generate a speciated Lookup as well as LookupMaybe method. + // The Lookup method returns nil in case of *either* an out-of-range/absent value or a null value, + // and so should only be used if the list type doesn't allow nullable keys or if the caller doesn't care about the difference. + // The LookupMaybe method returns a MaybeT type for the list value, + // and is needed if the list allows nullable values and the caller wishes to distinguish between null and out-of-range/absent. + // (The Lookup method should be preferred for lists that have non-nullable keys, because LookupMaybe may incur additional costs; + // boxing something into a maybe when it wasn't already stored that way costs an alloc(!), + // and may additionally incur a memcpy if the maybe for the value type doesn't use pointers internally). + doTemplate(` + func (n *_{{ .Type | TypeSymbol }}) Lookup(idx int) {{ .Type.ValueType | TypeSymbol }} { + if n.Length() <= idx { + return nil + } + v := &n.x[idx] + {{- if .Type.ValueIsNullable }} + if v.m == schema.Maybe_Null { + return nil + } + return {{ if not (MaybeUsesPtr .Type.ValueType) }}&{{end}}v.v + {{- else}} + return v + {{- end}} + } + func (n *_{{ .Type | TypeSymbol }}) LookupMaybe(idx int) Maybe{{ .Type.ValueType | TypeSymbol }} { + if n.Length() <= idx { + return nil + } + v := &n.x[idx] + {{- if .Type.ValueIsNullable }} + return v + {{- else}} + return &_{{ .Type.ValueType | TypeSymbol }}__Maybe{ + m: schema.Maybe_Value, + v: {{ if not (MaybeUsesPtr .Type.ValueType) }}*{{end}}v, + } + {{- end}} + } + + var _{{ .Type | TypeSymbol }}__valueAbsent = _{{ .Type.ValueType | TypeSymbol }}__Maybe{m:schema.Maybe_Absent} + `, w, g.AdjCfg, g) + + // Generate a speciated iterator. + // The main advantage of this over the general ipld.ListIterator is of course keeping types visible (and concrete, to the compiler's eyes in optimizations, too). + // It also elides the error return from the iterator's Next method. (Overreads will result in -1 as an index and nil values; this is both easily avoidable, and unambiguous if you do goof and hit it.) + doTemplate(` + func (n {{ .Type | TypeSymbol }}) Iterator() *{{ .Type | TypeSymbol }}__Itr { + return &{{ .Type | TypeSymbol }}__Itr{n, 0} + } + + type {{ .Type | TypeSymbol }}__Itr struct { + n {{ .Type | TypeSymbol }} + idx int + } + + func (itr *{{ .Type | TypeSymbol }}__Itr) Next() (idx int, v {{if .Type.ValueIsNullable }}Maybe{{end}}{{ .Type.ValueType | TypeSymbol }}) { + if itr.idx >= len(itr.n.x) { + return -1, nil + } + idx = itr.idx + v = &itr.n.x[itr.idx] + itr.idx++ + return + } + func (itr *{{ .Type | TypeSymbol }}__Itr) Done() bool { + return itr.idx >= len(itr.n.x) + } + + `, w, g.AdjCfg, g) } func (g listGenerator) EmitNativeBuilder(w io.Writer) {