Codegen various improvements #106
Conversation
No surprise this is needed; just filling in the gaps.
No suprise this is needed; just filling in the gaps.
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.)
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).
| // 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 }} { |
There was a problem hiding this comment.
thought on naming: i might more naturally think of this at Get rather than Lookup
There was a problem hiding this comment.
I thought about this for a surprisingly long time. At this point, I'd say either all the methods on the Node interface turn into Get* and this goes with them, or, none of them do. Having the gen'd methods with concrete types have a totally different name prefix than the generic interface methods was the one thing that was definitely a mistake.
|
A bit of a tangent, but - have you thought about code bloat in general? The current schema generator already outputs quite a lot of code, and we seem to be adding more and more, so I worry that this will result in binaries getting heavier and builds getting slower over time. One of the main reasons protobuf had a major breaking version last year, I seem to recall, was to remove a lot of non-essential cruft that wasn't really needed and was bloating projects. It cost them a lot of effort to break away from that, so I just want to make sure we're not headed for the same path. |
|
Yes. The size of output is currently pretty out of control. There's a massive amount of low-hanging fruit here. (I hope.) I've made no attempts to minimize it yet. It's getting to be time to make some efforts on that. (This'll be truly into tangent land, but, fwiw: I think the two biggest things to chase first will probably be: all the state machine switches around These iterators are definitely going to be worth it either way, though. I tried to write some code without them, and it was pretty horrendous and involved a lot of cast assertions, and the lack of autocomplete made for very poor UX. |
|
Merging; want to launch another PR which builds upon this, momentarily |
Could you open this as a separate issue, so I have a place to collect my thoughts alongside yours? |
|
New issue for collecting codegen output size notes: #108 |
Bulk PR for a bunch of individually minor codegen improvements.
It's basically a laundry list of small quality-of-life things that I encountered today while working on other things that consume codegen'd code.
(These aren't particularly interesting, so I'll probably merge this with or without review, but if anyone wants to give it a quick eyeball, have at it.)