A first stab at porting the XCB X11 protocol bindings to go.

A first stab at porting the XCB X11 protocol bindings to go. The python script needs a checkout of xcb/proto to generate an xproto.go file, which together with xgb.go provide functions to access all of the core X11 protocol requests. I have included the generated file. Extensions and authentication methods are not implemented.

[tor, 2009-11-29 22:43:16]

Hello. I spent a few evenings last week cooking up some X11 bindings for go based on XCB, figuring it would be something handy to have. The contributions page didn't say much on what sort of submissions you welcome, so if this is sent to the wrong place I apologize. -tor

[rsc, 2009-11-30 02:01:11]

Wow! This looks great; thanks for digging into this. Adding Nigel, who was working on an X11 interface by hand. I think the generated code is definitely a good approach. Nigel may have more comments, but in general this seems like a very reasonable and Go-like interface. I suspect that we'll want to eliminate the single-goroutine restriction eventually, but it's fine for now. I made a bunch of small comments below. I realize this is a work in progress ad I'm happy to check this in as a checkpoint whenever as long as the comment in the Makefile is addressed. Thanks again. (I see your CLA entry; thanks.) http://codereview.appspot.com/162053/diff/1019/1021 File src/pkg/xgb/Makefile (right): http://codereview.appspot.com/162053/diff/1019/1021#newcode14 src/pkg/xgb/Makefile:14: xproto.go: go_client.py proto/src/xproto.xml Is this going to complain if proto/src/xproto.xml does not exist? Please test that you can rm -rf proto and make still runs. http://codereview.appspot.com/162053/diff/1019/1022 File src/pkg/xgb/go_client.py (right): http://codereview.appspot.com/162053/diff/1019/1022#newcode1 src/pkg/xgb/go_client.py:1: #!/usr/bin/env python It would be interesting to see what this looks like in Go. I think the "xml" and "template" packages might make this even easier. I've certainly found xml simpler than xml.etree.cElementTree. That said, it's certainly ok to leave this as is. http://codereview.appspot.com/162053/diff/1019/1023 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/162053/diff/1019/1023#newcode16 src/pkg/xgb/xgb.go:16: // The connection to the server. This struct is not reentrant, so please Documentation comments should begin with the name of the thing being documented. See http://golang.org/doc/effective_go.html#commentary // A Conn represents a connection to the X server. // Only one goroutine should use a Conn's methods at a time. http://codereview.appspot.com/162053/diff/1019/1023#newcode19 src/pkg/xgb/xgb.go:19: sock	net.Conn; please call this conn (on some systems it might not be a socket). http://codereview.appspot.com/162053/diff/1019/1023#newcode24 src/pkg/xgb/xgb.go:24: error	os.Error; typically err no error. http://codereview.appspot.com/162053/diff/1019/1023#newcode43 src/pkg/xgb/xgb.go:43: func put8(buf []byte, offset int, v uint8)	{ buf[offset] = v } Instead of passing offset explicitly, you could just use slicing and make this put8(buf []byte, v uint8) etc. The call sites would change from put8(buf, offset, v) to put8(buf[offset:], v), etc. (for all the put/get) For the specific case of put8/get8, it looks like they are unused; delete? http://codereview.appspot.com/162053/diff/1019/1023#newcode77 src/pkg/xgb/xgb.go:77: y = ((y + (y >> 3)) & 030707070707) % 077; The % 077 trick might have made sense on a PDP-10, but it doesn't save much on today's machines. It's cute, but a for loop would be clearer and plenty efficient, since each one of these calls is balanced by a network operation and packet marshalling. http://codereview.appspot.com/162053/diff/1019/1023#newcode90 src/pkg/xgb/xgb.go:90: for i := q.a; i < q.b; i++ { If you update to the latest tree (cd $GOROOT/src; hg pull -u; ./all.bash) you can write this as copy(q.data, q.data[q.a:]) If the queue is typically going to have lots of elements in it, you could avoid the copying entirely by changing the queue to be type queue struct { data [][]byte; i int; // offset in data n int; // number of valid elements } but then the copy into newData below would have to be two copy() calls for the two halves so maybe that's not worth it. http://codereview.appspot.com/162053/diff/1019/1023#newcode96 src/pkg/xgb/xgb.go:96: for i := q.a; i < q.b; i++ { copy(newData, q.data); http://codereview.appspot.com/162053/diff/1019/1023#newcode106 src/pkg/xgb/xgb.go:106: func (q *queue) unqueue() []byte { the verb is usually dequeue. http://codereview.appspot.com/162053/diff/1019/1023#newcode115 src/pkg/xgb/xgb.go:115: // Send a request to the server and return its associated sequence number, or cookie. // sendRequest sends ... and returns ... http://codereview.appspot.com/162053/diff/1019/1023#newcode128 src/pkg/xgb/xgb.go:128: // Send enough bytes to align to a 4-byte border. // sendPadding sends .... // It is used to pad ... etc. (I won't mark each comment) http://codereview.appspot.com/162053/diff/1019/1023#newcode131 src/pkg/xgb/xgb.go:131: var three [3]byte; This allocates each time sendPadding is called. It probably doesn't matter, but if you move this into c *Conn as zero [3]byte and use c.sock.Write(c.zero[0:x]) then you avoid the allocation. http://codereview.appspot.com/162053/diff/1019/1023#newcode134 src/pkg/xgb/xgb.go:134: _, error := c.sock.Write(three[0:x]); Typically called err not error. (etc.) http://codereview.appspot.com/162053/diff/1019/1023#newcode175 src/pkg/xgb/xgb.go:175: if buf[0] == 0 { switch buf[0] { http://codereview.appspot.com/162053/diff/1019/1023#newcode184 src/pkg/xgb/xgb.go:184: return os.NewError(str); More idiomatic Go would be to put this information in a struct and use that as the error. type Error struct { Name string; Major uint8; Minor uint16; Cookie uint16; Id uint32; } func (e *Error) String() string { return fmt.Sprintf("Bad%s ...", ...) } and then here just do err := &Error{ Name: errorNames[buf[1]], Cookie: get16(buf[2:]), Id: get32(buf[4:]), Minor: get16(buf[8:]), Major: buf[10] }; fmt.Fprintf(os.Stderr, "x protocol error: %s\n", err); return err; http://codereview.appspot.com/162053/diff/1019/1023#newcode192 src/pkg/xgb/xgb.go:192: copy(bigbuf[0:32], buf); I see you found copy. On the next line you can drop the len(bigbuf). To fix all of these programmatically, you can run gofmt -w -r 'a[b:len(a)] -> a[b:]' xgb.go though it looks like maybe this is the only one. http://codereview.appspot.com/162053/diff/1019/1023#newcode215 src/pkg/xgb/xgb.go:215: reply, exists := c.replies[cookie]; exists is fine, but this is typically ok, and merged with the next line: if reply, ok := c.replies[cookie]; ok { c.replies[cookie] = nil, false; return reply, nil; } if err := c.readNextReply(); err != nil { return nil, err; } http://codereview.appspot.com/162053/diff/1019/1023#newcode234 src/pkg/xgb/xgb.go:234: return parseEvent(reply), nil It seems like if parseEvent fails, it should return an os.Error instead of an error masquerading as an Event. If you make parseEvent return (Event, os.Error) too, here you can do return parseEvent(reply) http://codereview.appspot.com/162053/diff/1019/1023#newcode256 src/pkg/xgb/xgb.go:256: // Return the Screen info for the default screen (0 or the one given in the DISPLAY environment variable). This mentions the DISPLAY environment variable but Dial doesn't seem to. s/DISPLAY environment variable/display string/? http://codereview.appspot.com/162053/diff/1019/1023#newcode290 src/pkg/xgb/xgb.go:290: var authName, authData []byte; // TODO: Fill these in? http://codereview.appspot.com/162053/diff/1019/1024 File src/pkg/xgb/xproto.go (right): http://codereview.appspot.com/162053/diff/1019/1024#newcode110 src/pkg/xgb/xproto.go:110: Depth	byte; This is an admittedly fine distinction, but we tend to use uint8 for small integers and byte for bytes (like the bytes of "hello"). Since depth is a number first, it should probably be uint8. This might be too much for the converter, in which case byte is fine. http://codereview.appspot.com/162053/diff/1019/1024#newcode122 src/pkg/xgb/xproto.go:122: // enum VisualClass I don't know how reasonable it is to do this, but the constants would typically be spelled using CamelCase in Go, and they could have their own type to avoid accidental uses as other things. Especially in the X protocol with all its many different constants, this seems useful: type VisualClass uint8; const ( VisualClassStaticGray VisualClass = iota; VisualClassGrayScale; VisualClassStaticColor; ... ) If all you have are the values in the xml, then the iota doesn't make sense but it may still make sense to put the type annotation on each line. http://codereview.appspot.com/162053/diff/1019/1024#newcode172 src/pkg/xgb/xproto.go:172: EVENT_MASK_NO_EVENT	= 0; Similarly type EventMask uint32 const ( EventMaskNoEvent EventMask = 0; EventMaskKeyPress EventMask = 1<<(iota-1); EventMaskKeyRelease; ... ) http://codereview.appspot.com/162053/diff/1019/1024#newcode4249 src/pkg/xgb/xproto.go:4249: } Wow, that generator sure did a lot of work!

[nigeltao, 2009-11-30 13:02:33]

http://codereview.appspot.com/162053/diff/1019/1023 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/162053/diff/1019/1023#newcode9 src/pkg/xgb/xgb.go:9: import "fmt" Russ -- is it conventional to have only one "import" and the package names within ()s? http://codereview.appspot.com/162053/diff/1019/1023#newcode22 src/pkg/xgb/xgb.go:22: replies	map[uint16][]byte; Russ -- would it be worth having type seqNum uint16; and type xID uint32; http://codereview.appspot.com/162053/diff/1019/1023#newcode33 src/pkg/xgb/xgb.go:33: // Generate a new unused ID for use with requests like CreateWindow. Can you add a TODO comment to check for ID overflow, as per libxcb's xcb_generate_id? http://codereview.appspot.com/162053/diff/1019/1023#newcode120 src/pkg/xgb/xgb.go:120: fmt.Fprintf(os.Stderr, "x protocol write error: %s\n", error); Nit: I think the "X" in "X protocol" should be capitalized. Also, I'm not sure if a library should be printing to stderr instead of just returning an os.Error to the caller somehow, but I'm not the definitive guide on go error handling. http://codereview.appspot.com/162053/diff/1019/1023#newcode167 src/pkg/xgb/xgb.go:167: func (c *Conn) readNextReply() os.Error { Since this method reads both replies and events, perhaps a better name is just readNext(). http://codereview.appspot.com/162053/diff/1019/1023#newcode259 src/pkg/xgb/xgb.go:259: // Connect to the X server given in the 'display' string. Is there an example main.go program to show me how to use this? I'm confused as to what display string to pass to Dial -- it's not ":0.0", right? http://codereview.appspot.com/162053/diff/1019/1023#newcode291 src/pkg/xgb/xgb.go:291: var buf []byte; I would fold this line with the next one using the := short variable declaration. Specifically: buf := make(etcetera); http://codereview.appspot.com/162053/diff/1019/1023#newcode353 src/pkg/xgb/xgb.go:353: type ClientMessageData struct /*union */ { I'd drop the /*union*/, since you've already made your point in the comment above, but Russ would have a better notion of Go style. http://codereview.appspot.com/162053/diff/1019/1024 File src/pkg/xgb/xproto.go (right): http://codereview.appspot.com/162053/diff/1019/1024#newcode1348 src/pkg/xgb/xproto.go:1348: b := make([]byte, 8); This question for Russ rather than Tor: IIUC, this line (and many similar ones for each of the other X11 requests) allocates a new buffer, which will be garbage collected at the end of the method call. An alternative is to have a sufficiently large buffer as a member of the Conn struct (c.f. the buf member in pkg/exp/draw/x11/conn.go), and re-use it for each call. Would it be worth doing something similar in the (generated) xproto.go?

[tor, 2009-11-30 13:48:45]

In parseEvent and friends I return the struct by value, boxed into an interface. I don't know if this is preferred to returning a pointer, but it made the type switch look nicer. I have considered wrapping the connection with a buffered reader/writer, since there can be a lot of small reads and writes. That would require flushing in readNext. When it comes to extensions, the only one I really care about is Shm, for XShmPutImage so that image data doesn't have to be pumped over the socket. I haven't seen an mmap() call exposed in the os package, but you use it internally in the runtime for malloc. Eventually we will want a solution to this, especially for doing client side rendering in draw. http://codereview.appspot.com/162053/diff/1019/1021 File src/pkg/xgb/Makefile (right): http://codereview.appspot.com/162053/diff/1019/1021#newcode14 src/pkg/xgb/Makefile:14: xproto.go: go_client.py proto/src/xproto.xml On 2009/11/30 02:01:11, rsc wrote: > Is this going to complain if proto/src/xproto.xml > does not exist? Please test that you can rm -rf proto > and make still runs. It will try to do a git checkout... I am unsure of the legal issues involved if we want to just swallow the xcb/proto package. If I do rewrite the generating tool then all we'd need are the xml files (xproto.xml primarily). http://codereview.appspot.com/162053/diff/1019/1022 File src/pkg/xgb/go_client.py (right): http://codereview.appspot.com/162053/diff/1019/1022#newcode1 src/pkg/xgb/go_client.py:1: #!/usr/bin/env python On 2009/11/30 02:01:11, rsc wrote: > It would be interesting to see what this looks like in Go. > I think the "xml" and "template" packages might make > this even easier. I've certainly found xml simpler than > xml.etree.cElementTree. > > That said, it's certainly ok to leave this as is. I just took a look at the xml package and it sure looks neat. I'll take a look and see what I can do, it would be an interesting exercise and this script is rather ugly and inflexible. http://codereview.appspot.com/162053/diff/1019/1023 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/162053/diff/1019/1023#newcode16 src/pkg/xgb/xgb.go:16: // The connection to the server. This struct is not reentrant, so please On 2009/11/30 02:01:11, rsc wrote: > Documentation comments should begin with the name > of the thing being documented. See > http://golang.org/doc/effective_go.html#commentary > > // A Conn represents a connection to the X server. > // Only one goroutine should use a Conn's methods at a time. > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode19 src/pkg/xgb/xgb.go:19: sock	net.Conn; On 2009/11/30 02:01:11, rsc wrote: > please call this conn > (on some systems it might not be a socket). > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode22 src/pkg/xgb/xgb.go:22: replies	map[uint16][]byte; On 2009/11/30 13:02:33, nigeltao_golang wrote: > Russ -- would it be worth having > type seqNum uint16; > and > type xID uint32; I decided against using too many type aliases, especially the Drawable/Pixmap/Window stuff that would require a lot of annoying typecasts on the user side of the interface. Having the types makes sense too, and the drawbacks won't be so bad if we use Xid instead of Window/Drawable/Pixmap and Gc/Font/Fontable. http://codereview.appspot.com/162053/diff/1019/1023#newcode24 src/pkg/xgb/xgb.go:24: error	os.Error; On 2009/11/30 02:01:11, rsc wrote: > typically err no error. > I've been writing error and okay for five years in my other projects. I generally prefer pronouncable and readable names, especially when they don't conflict with brevity. However, if err is the tradition in go, then so be it. http://codereview.appspot.com/162053/diff/1019/1023#newcode33 src/pkg/xgb/xgb.go:33: // Generate a new unused ID for use with requests like CreateWindow. On 2009/11/30 13:02:33, nigeltao_golang wrote: > Can you add a TODO comment to check for ID overflow, as per libxcb's > xcb_generate_id? What should the behavior be when we run out? http://codereview.appspot.com/162053/diff/1019/1023#newcode43 src/pkg/xgb/xgb.go:43: func put8(buf []byte, offset int, v uint8)	{ buf[offset] = v } On 2009/11/30 02:01:11, rsc wrote: > Instead of passing offset explicitly, you could just use > slicing and make this put8(buf []byte, v uint8) etc. > The call sites would change from put8(buf, offset, v) > to put8(buf[offset:], v), etc. (for all the put/get) Good idea, but I could also make the auto generated code do the bit shuffling directly. > For the specific case of put8/get8, it looks like they > are unused; delete? They're used in the generated code. I was lazy: "put%d". http://codereview.appspot.com/162053/diff/1019/1023#newcode77 src/pkg/xgb/xgb.go:77: y = ((y + (y >> 3)) & 030707070707) % 077; On 2009/11/30 02:01:11, rsc wrote: > The % 077 trick might have made sense on a PDP-10, > but it doesn't save much on today's machines. > It's cute, but a for loop would be clearer and plenty > efficient, since each one of these calls is balanced > by a network operation and packet marshalling. > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode90 src/pkg/xgb/xgb.go:90: for i := q.a; i < q.b; i++ { On 2009/11/30 02:01:11, rsc wrote: > If you update to the latest tree > (cd $GOROOT/src; hg pull -u; ./all.bash) > you can write this as > > copy(q.data, q.data[q.a:]) > > If the queue is typically going to have lots > of elements in it, you could avoid the copying > entirely by changing the queue to be > > type queue struct { > data [][]byte; > i int; // offset in data > n int; // number of valid elements > } > > but then the copy into newData below would > have to be two copy() calls for the two halves > so maybe that's not worth it. Or I could just have a and b wrap around len(data) with %. But that would involve actual thinking, and I'd get it wrong :) Fixed the other stuff. http://codereview.appspot.com/162053/diff/1019/1023#newcode96 src/pkg/xgb/xgb.go:96: for i := q.a; i < q.b; i++ { On 2009/11/30 02:01:11, rsc wrote: > copy(newData, q.data); > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode106 src/pkg/xgb/xgb.go:106: func (q *queue) unqueue() []byte { On 2009/11/30 02:01:11, rsc wrote: > the verb is usually dequeue. > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode115 src/pkg/xgb/xgb.go:115: // Send a request to the server and return its associated sequence number, or cookie. On 2009/11/30 02:01:11, rsc wrote: > // sendRequest sends ... and returns ... > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode128 src/pkg/xgb/xgb.go:128: // Send enough bytes to align to a 4-byte border. On 2009/11/30 02:01:11, rsc wrote: > // sendPadding sends .... > // It is used to pad ... > > etc. (I won't mark each comment) > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode131 src/pkg/xgb/xgb.go:131: var three [3]byte; On 2009/11/30 02:01:11, rsc wrote: > This allocates each time sendPadding is called. > It probably doesn't matter, but if you move > this into c *Conn as zero [3]byte > and use c.sock.Write(c.zero[0:x]) then > you avoid the allocation. Ow. I just assumed that since it was a constant size array and it wasn't created by new, make or & it would be stack allocated. http://codereview.appspot.com/162053/diff/1019/1023#newcode134 src/pkg/xgb/xgb.go:134: _, error := c.sock.Write(three[0:x]); On 2009/11/30 02:01:11, rsc wrote: > Typically called err not error. (etc.) > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode175 src/pkg/xgb/xgb.go:175: if buf[0] == 0 { On 2009/11/30 02:01:11, rsc wrote: > switch buf[0] { > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode184 src/pkg/xgb/xgb.go:184: return os.NewError(str); On 2009/11/30 02:01:11, rsc wrote: > More idiomatic Go would be to put this information > in a struct and use that as the error. > > type Error struct { > Name string; > Major uint8; > Minor uint16; > Cookie uint16; > Id uint32; > } > > func (e *Error) String() string { > return fmt.Sprintf("Bad%s ...", ...) > } > > and then here just do > > err := &Error{ > Name: errorNames[buf[1]], > Cookie: get16(buf[2:]), > Id: get32(buf[4:]), > Minor: get16(buf[8:]), > Major: buf[10] > }; > fmt.Fprintf(os.Stderr, "x protocol error: %s\n", err); > return err; > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode192 src/pkg/xgb/xgb.go:192: copy(bigbuf[0:32], buf); On 2009/11/30 02:01:11, rsc wrote: > I see you found copy. On the next line you can drop > the len(bigbuf). To fix all of these programmatically, > you can run > > gofmt -w -r 'a[b:len(a)] -> a[b:]' xgb.go > > though it looks like maybe this is the only one. > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode215 src/pkg/xgb/xgb.go:215: reply, exists := c.replies[cookie]; On 2009/11/30 02:01:11, rsc wrote: > exists is fine, but this is typically ok, and merged > with the next line: > > if reply, ok := c.replies[cookie]; ok { > c.replies[cookie] = nil, false; > return reply, nil; > } > if err := c.readNextReply(); err != nil { > return nil, err; > } > I generally dislike putting more than one concept/instruction/whatsthename on each line, like the merged call and test idiom in the readNextReply statement. http://codereview.appspot.com/162053/diff/1019/1023#newcode234 src/pkg/xgb/xgb.go:234: return parseEvent(reply), nil On 2009/11/30 02:01:11, rsc wrote: > It seems like if parseEvent fails, it should return an > os.Error instead of an error masquerading as an Event. > If you make parseEvent return (Event, os.Error) too, > here you can do > > return parseEvent(reply) > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode256 src/pkg/xgb/xgb.go:256: // Return the Screen info for the default screen (0 or the one given in the DISPLAY environment variable). On 2009/11/30 02:01:11, rsc wrote: > This mentions the DISPLAY environment variable > but Dial doesn't seem to. > > s/DISPLAY environment variable/display string/? > Done. http://codereview.appspot.com/162053/diff/1019/1023#newcode259 src/pkg/xgb/xgb.go:259: // Connect to the X server given in the 'display' string. On 2009/11/30 13:02:33, nigeltao_golang wrote: > Is there an example main.go program to show me how to use this? I'm confused as > to what display string to pass to Dial -- it's not ":0.0", right? See my darcs repo for an example program: http://ghostscript.com/~tor/repos/xgb/main.go http://codereview.appspot.com/162053/diff/1019/1023#newcode290 src/pkg/xgb/xgb.go:290: var authName, authData []byte; On 2009/11/30 02:01:11, rsc wrote: > // TODO: Fill these in? > Indeed. It's one of the things I haven't done yet, reading the magic cookies from .Xauthority. http://codereview.appspot.com/162053/diff/1019/1023#newcode291 src/pkg/xgb/xgb.go:291: var buf []byte; On 2009/11/30 13:02:33, nigeltao_golang wrote: > I would fold this line with the next one using the := short variable > declaration. Specifically: > buf := make(etcetera); I put it separately because I reuse the variable for another buffer further down. http://codereview.appspot.com/162053/diff/1019/1023#newcode353 src/pkg/xgb/xgb.go:353: type ClientMessageData struct /*union */ { On 2009/11/30 13:02:33, nigeltao_golang wrote: > I'd drop the /*union*/, since you've already made your point in the comment > above, but Russ would have a better notion of Go style. Done. http://codereview.appspot.com/162053/diff/1019/1024 File src/pkg/xgb/xproto.go (right): http://codereview.appspot.com/162053/diff/1019/1024#newcode110 src/pkg/xgb/xproto.go:110: Depth	byte; On 2009/11/30 02:01:11, rsc wrote: > This is an admittedly fine distinction, > but we tend to use uint8 for small integers > and byte for bytes (like the bytes of "hello"). > Since depth is a number first, it should > probably be uint8. This might be too much > for the converter, in which case byte is fine. The information is in the XML file but has disappeared by the time the proto/xcbgen code is done with it. Should I rewrite the generation code in go I can certainly do this. http://codereview.appspot.com/162053/diff/1019/1024#newcode122 src/pkg/xgb/xproto.go:122: // enum VisualClass On 2009/11/30 02:01:11, rsc wrote: > I don't know how reasonable it is to do this, but > the constants would typically be spelled using CamelCase > in Go, and they could have their own type to avoid > accidental uses as other things. Especially in the X protocol > with all its many different constants, this seems useful: I've seen CAPS in the os package and thought that was the standard. I'd be happy to camel-casify it (well, not too fond of camel case in general but that's a discussion for another time). > type VisualClass uint8; > const ( > VisualClassStaticGray VisualClass = iota; > VisualClassGrayScale; > VisualClassStaticColor; > ... > ) > > If all you have are the values in the xml, then the iota > doesn't make sense but it may still make sense to > put the type annotation on each line. > Likewise the enum info is gone (or I just didn't see it) by the time my python script gets it. Since consts have no type I don't see how the enum types would be useful (other than for godoc, and that's dubious).

[rsc, 2009-11-30 17:16:21]

couple of replies. please run hg upload 162053 at some point so we can see the changes you made (and commit them once they're ready). http://codereview.appspot.com/162053/diff/1019/1021 File src/pkg/xgb/Makefile (right): http://codereview.appspot.com/162053/diff/1019/1021#newcode14 src/pkg/xgb/Makefile:14: xproto.go: go_client.py proto/src/xproto.xml On 2009/11/30 13:48:46, tor wrote: > On 2009/11/30 02:01:11, rsc wrote: > > Is this going to complain if proto/src/xproto.xml > > does not exist? Please test that you can rm -rf proto > > and make still runs. > > It will try to do a git checkout... > > I am unsure of the legal issues involved if we want to just > swallow the xcb/proto package. If I do rewrite the generating > tool then all we'd need are the xml files (xproto.xml primarily). I think it's fine to have the code in the makefile to regenerate, but it shouldn't happen on every build, since these files don't change very often and then we avoid needing to have a copy of the xcb files. If you change the target name from xproto.go to something else (say, xproto), then people can still run "make xproto" to update but the normal build won't bother. That seems like a good balance. It's pretty close to what we do in the unicode package. If you do rewrite the generator as a pure Go program (which can wait until later) then it might even be able to fetch the XML from a git web server instead of needing the local git checkout. The unicode maketables.go just reads its files right off the unicode web site. Anyway, this can certainly wait; what you've got here is a good checkpoint. http://codereview.appspot.com/162053/diff/1019/1023 File src/pkg/xgb/xgb.go (right): http://codereview.appspot.com/162053/diff/1019/1023#newcode9 src/pkg/xgb/xgb.go:9: import "fmt" On 2009/11/30 13:02:33, nigeltao_golang wrote: > Russ -- is it conventional to have only one "import" and the package names > within ()s? Good catch - this import list should be factored. import ( "fmt"; "os"; ... ) and sorted http://codereview.appspot.com/162053/diff/1019/1023#newcode22 src/pkg/xgb/xgb.go:22: replies	map[uint16][]byte; On 2009/11/30 13:48:46, tor wrote: > On 2009/11/30 13:02:33, nigeltao_golang wrote: > > Russ -- would it be worth having > > type seqNum uint16; > > and > > type xID uint32; > > I decided against using too many type aliases, especially the > Drawable/Pixmap/Window stuff that would require a lot of > annoying typecasts on the user side of the interface. Having > the types makes sense too, and the drawbacks won't be so bad > if we use Xid instead of Window/Drawable/Pixmap and Gc/Font/Fontable. This seems like a good compromise: it's always confused me to have to use all those different types in the core X libraries, but separating ids from other integers would be a good thing and shouldn't cause many casts. The package already has xgb in the name so there's no need for a leading X. type Id uint32 would mean clients see xgb.Id which is pretty good. http://codereview.appspot.com/162053/diff/1019/1023#newcode24 src/pkg/xgb/xgb.go:24: error	os.Error; On 2009/11/30 13:48:46, tor wrote: > On 2009/11/30 02:01:11, rsc wrote: > > typically err no error. > > > > I've been writing error and okay for five years in my other projects. I > generally prefer pronouncable and readable names, especially when they don't > conflict with brevity. However, if err is the tradition in go, then so be it. I certainly understand the consistency in your other projects, but the consistency here is err, ok. http://codereview.appspot.com/162053/diff/1019/1023#newcode33 src/pkg/xgb/xgb.go:33: // Generate a new unused ID for use with requests like CreateWindow. On 2009/11/30 13:48:46, tor wrote: > On 2009/11/30 13:02:33, nigeltao_golang wrote: > > Can you add a TODO comment to check for ID overflow, as per libxcb's > > xcb_generate_id? > > What should the behavior be when we run out? That's why it's a TODO, I guess. ;-) // TODO(tor,nigeltao): Handle id overflow. http://codereview.appspot.com/162053/diff/1019/1023#newcode131 src/pkg/xgb/xgb.go:131: var three [3]byte; On 2009/11/30 13:48:46, tor wrote: > On 2009/11/30 02:01:11, rsc wrote: > > This allocates each time sendPadding is called. > > It probably doesn't matter, but if you move > > this into c *Conn as zero [3]byte > > and use c.sock.Write(c.zero[0:x]) then > > you avoid the allocation. > > Ow. I just assumed that since it was a constant size array and it wasn't created > by new, make or & it would be stack allocated. If you take its address and the compiler isn't sure that the reference won't last past the function call, it goes on the stack. There's no way for the compiler to know whether Write is going to squirrel the pointer away for later. http://codereview.appspot.com/162053/diff/1019/1023#newcode215 src/pkg/xgb/xgb.go:215: reply, exists := c.replies[cookie]; On 2009/11/30 13:48:46, tor wrote: > On 2009/11/30 02:01:11, rsc wrote: > > exists is fine, but this is typically ok, and merged > > with the next line: > > > > if reply, ok := c.replies[cookie]; ok { > > c.replies[cookie] = nil, false; > > return reply, nil; > > } > > if err := c.readNextReply(); err != nil { > > return nil, err; > > } > > > > I generally dislike putting more than one concept/instruction/whatsthename on > each line, like the merged call and test idiom in the readNextReply statement. I'm sympathetic to that but it's still a common Go idiom and looks out of place here not to merge them. http://codereview.appspot.com/162053/diff/1019/1023#newcode291 src/pkg/xgb/xgb.go:291: var buf []byte; On 2009/11/30 13:48:46, tor wrote: > On 2009/11/30 13:02:33, nigeltao_golang wrote: > > I would fold this line with the next one using the := short variable > > declaration. Specifically: > > buf := make(etcetera); > > I put it separately because I reuse the variable for another buffer further > down. You can still use := though. It's pretty unusual to see var declarations in a function body in Go. http://codereview.appspot.com/162053/diff/1019/1024 File src/pkg/xgb/xproto.go (right): http://codereview.appspot.com/162053/diff/1019/1024#newcode110 src/pkg/xgb/xproto.go:110: Depth	byte; On 2009/11/30 13:48:46, tor wrote: > On 2009/11/30 02:01:11, rsc wrote: > > This is an admittedly fine distinction, > > but we tend to use uint8 for small integers > > and byte for bytes (like the bytes of "hello"). > > Since depth is a number first, it should > > probably be uint8. This might be too much > > for the converter, in which case byte is fine. > > The information is in the XML file but has disappeared > by the time the proto/xcbgen code is done with it. > Should I rewrite the generation code in go I can certainly do this. Sure. It's probably not worth rewriting the generation code in Go in this CL. If you're interested in doing that later, that'd be great. http://codereview.appspot.com/162053/diff/1019/1024#newcode122 src/pkg/xgb/xproto.go:122: // enum VisualClass The caps in the OS package are kind of grandfathered in because they are so common (O_RDONLY, EINVAL, etc) but words separated by underscores should become camel case in Go. I wasn't wild about it at first but like any convention you get used to it. > Likewise the enum info is gone (or I just didn't see it) by the > time my python script gets it. Since consts have no type I don't > see how the enum types would be useful (other than for godoc, > and that's dubious). Constants can have types, they just don't have to by default. So if you have type VisualClass uint8 const ( VisualClassStaticGray VisualClass = 0; VisualClassGrayScale VisualClass = 1; ... ) type ImageOrder uint8 const ( ImageOrderLsbFirst ImageOrder = 0; ImageOrderMsbFirst ImageOrder = 1; ) var x ImageByteOrder; and then you accidentally write x = VisualClassStaticGray, the compiler will complain. Same goes if x is a function argument or a struct field. Given the large number of enumerations that clients have to manage, this seems like it would be a win if you have the type information when laying down the struct fields. (I don't know whether you do.) http://codereview.appspot.com/162053/diff/1019/1024#newcode1348 src/pkg/xgb/xproto.go:1348: b := make([]byte, 8); On 2009/11/30 13:02:33, nigeltao_golang wrote: > This question for Russ rather than Tor: > > IIUC, this line (and many similar ones for each of the other X11 requests) > allocates a new buffer, which will be garbage collected at the end of the method > call. An alternative is to have a sufficiently large buffer as a member of the > Conn struct (c.f. the buf member in pkg/exp/draw/x11/conn.go), and re-use it for > each call. Would it be worth doing something similar in the (generated) > xproto.go? That's probably a good idea. I don't mind if it happens later (just add a TODO comment).

[tor, 2009-11-30 20:46:20]

Alright, I believe I have addressed most of the comments given and I added my simple example main function so you can see how the client side looks.

[rsc, 2009-11-30 20:55:32]

looks great. please change the few nits below and then i'll submit it. thanks again - i'm pretty excited to have this http://andi.latest.codereview.appspot.com/162053/diff/3018/2011 File src/pkg/Makefile (right): http://andi.latest.codereview.appspot.com/162053/diff/3018/2011#newcode101 src/pkg/Makefile:101: xgb\ Please run ./all.bash again in $GOROOT/src and make sure it succeeds. I suspect you need to add xgb to the NOTEST list below, since there are no unit tests in the package. http://andi.latest.codereview.appspot.com/162053/diff/3018/2012 File src/pkg/xgb/Makefile (right): http://andi.latest.codereview.appspot.com/162053/diff/3018/2012#newcode5 src/pkg/xgb/Makefile:5: include $(GOROOT)/src/Make.$(GOARCH) Please change to include ../../Make.$(GOARCH) and similarly below include ../../Make.pkg this is a recent change to make make happy when $(GOROOT) contains spaces. http://andi.latest.codereview.appspot.com/162053/diff/3018/2013 File src/pkg/xgb/example.go (right): http://andi.latest.codereview.appspot.com/162053/diff/3018/2013#newcode1 src/pkg/xgb/example.go:1: package main please add usual copyright header http://andi.latest.codereview.appspot.com/162053/diff/3018/2013#newcode4 src/pkg/xgb/example.go:4: "os"; sort list http://andi.latest.codereview.appspot.com/162053/diff/3018/2013#newcode10 src/pkg/xgb/example.go:10: c, error := xgb.Dial(os.Getenv("DISPLAY")); ,s/error/err/g http://andi.latest.codereview.appspot.com/162053/diff/3018/2015 File src/pkg/xgb/xgb.go (right): http://andi.latest.codereview.appspot.com/162053/diff/3018/2015#newcode373 src/pkg/xgb/xgb.go:373: // Close the connection. // Close closes the connection to the X server. http://andi.latest.codereview.appspot.com/162053/diff/3018/2015#newcode381 src/pkg/xgb/xgb.go:381: // Go doesn't have unions so we duplicate the data in all ClientMessageData holds the data from a client message, duplicated in three forms because Go doesn't have unions.

[rsc, 2009-11-30 22:20:12]

LGTM thanks very much. i'm about to submit this. i made one change: removed the 4-line copyright header from the autogenerated xproto.go and the lines in go_client.py that printed it. you'll want to do the same before running "hg sync" to avoid a merge conflict.

[rsc, 2009-11-30 22:25:54]

*** Submitted as http://code.google.com/p/go/source/detail?r=791149865a09 *** A first stab at porting the XCB X11 protocol bindings to go. The python script needs a checkout of xcb/proto to generate an xproto.go file, which together with xgb.go provide functions to access all of the core X11 protocol requests. I have included the generated file. Extensions and authentication methods are not implemented. R=r, rsc, nigeltao_golang http://codereview.appspot.com/162053 Committer: Russ Cox <rsc@golang.org>

[nigeltao_gnome, 2009-12-01 00:00:22]

2009/12/1 <tor.andersson@gmail.com>: > See my darcs repo for an example program: > > http://ghostscript.com/~tor/repos/xgb/main.go I presume you mean example.go, not main.go. I'm having trouble running the program. Any suggestions? $ echo $DISPLAY :0.0 $ ./8.out cannot connect: dial tcp :6000: connection refused cannot connect: dial tcp :6000: connection refused I'm on Ubuntu 8.04 "Hardy".