Skip to content
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

Please review my optimizations and code generation fixes #61

Closed
wants to merge 18 commits into from
Closed

Please review my optimizations and code generation fixes #61

wants to merge 18 commits into from

Conversation

apesternikov
Copy link

library:

  • thrift's BYTE type is signed (so it is int8, not byte)
  • asynchronous protocol is removed as irrelevant in go
    code generation:
  • safer constants using typed vars
  • int64 consts over 2^32 dont blow up
  • negative byte consts
  • generated struct doesn't include type metainformation field to avoid metainfo creating for every instance
  • map, list, set initializers didn't work
    tests:
  • use generated Work and CalculateArgs

library:
- thrift's BYTE type is signed (so it is int8, not byte)
- asynchronous protocol is removed as irrelevant in go
code generation:
- safer constants using typed vars
- int64 consts over 2^32 dont blow up
- negative byte consts
- generated struct doesn't include type metainformation field to avoid metainfo creating for every instance
- map, list, set initializers didn't work
tests:
- use generated Work and CalculateArgs
@pomack
Copy link
Owner

pomack commented Feb 19, 2013

Wow, thanks Aleksey! I don't have time to look through this for the next couple of weeks and I definitely want to take a good look to understand all the changes. I'll try to find some time at the beginning of March.

@apesternikov
Copy link
Author

sure. I hope I will have my other planned things done by that time. (I started short summary here https://github.com/apesternikov/thrift4go/wiki/Roadmap)

Aleksey Pesternikov and others added 5 commits February 19, 2013 12:58
lists are native (TList -> []elemtype)
maps are native (TMap -> map[keytype]valtype) native types including binary are supported as key type
sets are native (TSet -> map[valtype]bool
optimized struct's Read function
library:
removed types not present in thrift protocol spec
removed Tlist, TSet, TMap, TCompare, all data coercing fun from ttype.go
added TDebugProtocol
ReadFieldN -> readFieldN
remove ReadFieldFIELDNAME
WriteFieldN -> writeFieldN
remove WriteFieldFIELDNAME
@sdming
Copy link

sdming commented Mar 12, 2013

it's cool, I tried apesternikov/thrift4go, but i got error when generate code for Hbase thrift api, it stoped at code:
type TRowResult struct {
Row Text "row"; // 1

@sdming
Copy link

sdming commented Mar 12, 2013

bug of TlLIst, it break when i=0

if len(p.l) >= i {
p.l[i] = data
} else {
p.l = append(p.l, data)
}

@apesternikov
Copy link
Author

sdming,
compiler is fixed, it would be great if you try and tell us how it works.

@sdming
Copy link

sdming commented Mar 13, 2013

still can not compile Hbase thrift interface

error happen at the logic to handle key of map

you can download hbase thrift interface from http://wiki.apache.org/hadoop/Hbase/ThriftApi

@sdming
Copy link

sdming commented Mar 13, 2013

type Text []byte

you make a map map[string]XXX

but you set value with
map[Text] = xxx

@apesternikov
Copy link
Author

sorry, I forgot to commit my last changes. please test

@sdming
Copy link

sdming commented Mar 18, 2013

still many errors

an example

type TRegionInfo struct {
...
Version int8 "version" // 5
...
}

func (p *TRegionInfo) readField5(iprot thrift.TProtocol) {
...
v30, err31 := iprot.ReadByte()
p.Version = v30
...
}

...
cannot use v30 (type byte) as type int8 in assignment
...

@apesternikov
Copy link
Author

sdming, please make sure that your library is up to date. please update it with
go get -u github.com/apesternikov/thrift4go/lib/go/src/thrift

ReadByte() returns int8:
https://github.com/apesternikov/thrift4go/blob/master/lib/go/src/thrift/tprotocol.go#L65

and this is one of the changes from the original library:
https://github.com/pomack/thrift4go/blob/master/lib/go/src/thrift/tprotocol.go#L65

@sdming
Copy link

sdming commented Mar 19, 2013

it's strange, go get -u does not work, but git clone works fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants