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

src/pkg/xgb/xgb.go

+// Copyright 2009 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// The XGB package implements the X11 core protocol.
+// It is based on XCB: http://xcb.freedesktop.org/
+package xgb
+
+import "fmt"

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

Russ -- is it conventional to have only one "import" and the package names within ()s? 

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

Good catch - this import list should be factored. import ( "fmt"; "os"; ... ) and sorted 

+import "os"
+import "io"
+import "net"
+import "strings"
+import "strconv"
+
+// The connection to the server. This struct is not reentrant, so please

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

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. 

[tor, 2009/11/30 13:48:46]

Done. 

+// only call its methods from within one goroutine.
+type Conn struct {
+sock net.Conn;

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

please call this conn (on some systems it might not be a socket). 

[tor, 2009/11/30 13:48:46]

Done. 

+nextId uint32;
+nextCookie uint16;
+replies map[uint16][]byte;

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

Russ -- would it be worth having type seqNum uint16; and type xID uint32; 

[tor, 2009/11/30 13:48:46]

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. 

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

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. 

+events queue;
+error os.Error;

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

typically err no error. 

[tor, 2009/11/30 13:48:46]

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. 

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

I certainly understand the consistency in your other projects, but the consistency here is err, ok. 

+defaultScreen int;
+Setup SetupInfo;
+}
+
+// An interface that can contain any of the events returned by the server.
+// Use a type assertion switch to extract the Event structs.
+type Event interface{}
+
+// Generate a new unused ID for use with requests like CreateWindow.

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

Can you add a TODO comment to check for ID overflow, as per libxcb's xcb_generate_id? 

[tor, 2009/11/30 13:48:46]

What should the behavior be when we run out? 

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

That's why it's a TODO, I guess. ;-) // TODO(tor,nigeltao): Handle id overflow. 

+func (c *Conn) NewId() uint32 {
+id := c.nextId;
+c.nextId++;
+return id;
+}
+
+// Pad a length to align on 4 bytes.
+func pad(n int) int { return (n + 3) & ^3 }
+
+func put8(buf []byte, offset int, v uint8) { buf[offset] = v }

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

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? 

[tor, 2009/11/30 13:48:46]

Good idea, but I could also make the auto generated code do the bit shuffling directly. 
They're used in the generated code. I was lazy: "put%d". 

+func get8(buf []byte, offset int) uint8 { return buf[offset] }
+
+func put16(buf []byte, offset int, v uint16) {
+buf[offset] = byte(v);
+buf[offset+1] = byte(v >> 8);
+}
+
+func put32(buf []byte, offset int, v uint32) {
+buf[offset] = byte(v);
+buf[offset+1] = byte(v >> 8);
+buf[offset+2] = byte(v >> 16);
+buf[offset+3] = byte(v >> 24);
+}
+
+func get16(buf []byte, offset int) uint16 {
+v := uint16(buf[offset]);
+v |= uint16(buf[offset+1]) << 8;
+return v;
+}
+
+func get32(buf []byte, offset int) uint32 {
+v := uint32(buf[offset]);
+v |= uint32(buf[offset+1]) << 8;
+v |= uint32(buf[offset+2]) << 16;
+v |= uint32(buf[offset+3]) << 32;
+return v;
+}
+
+// Voodoo to count the number of bits set in a value list mask.
+func popCount(mask0 int) int {
+mask := uint32(mask0);
+y := (mask >> 1) & 033333333333;
+y = mask - y - ((y >> 1) & 033333333333);
+y = ((y + (y >> 3)) & 030707070707) % 077;

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

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. 

[tor, 2009/11/30 13:48:46]

Done. 

+return int(y);
+}
+
+// A simple queue used to stow away events.
+type queue struct {
+data [][]byte;
+a, b int;
+}
+
+func (q *queue) queue(item []byte) {
+if q.b == len(q.data) {
+if q.a > 0 {
+for i := q.a; i < q.b; i++ {

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

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. 

[tor, 2009/11/30 13:48:46]

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. 

+q.data[i-q.a] = q.data[i]
+}
+q.a, q.b = 0, q.b-q.a;
+} else {
+newData := make([][]byte, (len(q.data)*3)/2);
+for i := q.a; i < q.b; i++ {

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

copy(newData, q.data); 

[tor, 2009/11/30 13:48:46]

Done. 

+newData[i] = q.data[i]
+}
+q.data = newData;
+}
+}
+q.data[q.b] = item;
+q.b++;
+}
+
+func (q *queue) unqueue() []byte {

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

the verb is usually dequeue. 

[tor, 2009/11/30 13:48:46]

Done. 

+if q.a < q.b {
+item := q.data[q.a];
+q.a++;
+return item;
+}
+return nil;
+}
+
+// Send a request to the server and return its associated sequence number, or co okie.

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

// sendRequest sends ... and returns ... 

[tor, 2009/11/30 13:48:46]

Done. 

+// This will only send the fixed length portion of the request.
+func (c *Conn) sendRequest(buf []byte) uint16 {
+_, error := c.sock.Write(buf);
+if error != nil {
+fmt.Fprintf(os.Stderr, "x protocol write error: %s\n", error);

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

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. 

+c.error = error;
+}
+cookie := c.nextCookie;
+c.nextCookie++;
+return cookie;
+}
+
+// Send enough bytes to align to a 4-byte border.

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

// sendPadding sends .... // It is used to pad ... etc. (I won't mark each comment) 

[tor, 2009/11/30 13:48:46]

Done. 

+// Used to pad the variable length data that is used with some requests.
+func (c *Conn) sendPadding(n int) {
+var three [3]byte;

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

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. 

[tor, 2009/11/30 13:48:46]

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. 

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

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. 

+x := pad(n) - n;
+if x > 0 {
+_, error := c.sock.Write(three[0:x]);

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

Typically called err not error. (etc.) 

[tor, 2009/11/30 13:48:46]

Done. 

+if error != nil {
+fmt.Fprintf(os.Stderr, "x protocol write error: %s\n", e rror);
+c.error = error;
+}
+}
+}
+
+// Send a byte slice as variable length data after the fixed portion of a reques t.
+// Automatically pads.
+func (c *Conn) sendBytes(buf []byte) {
+_, error := c.sock.Write(buf);
+if error != nil {
+fmt.Fprintf(os.Stderr, "x protocol write error: %s\n", error);
+c.error = error;
+}
+c.sendPadding(len(buf));
+}
+
+func (c *Conn) sendString(str string) { c.sendBytes(strings.Bytes(str)) }
+
+// Send a list of 32-bit integers as variable length data.
+func (c *Conn) sendUInt32s(list []uint32) {
+buf := make([]byte, len(list)*4);
+for i := 0; i < len(list); i++ {
+put32(buf, i*4, list[i])
+}
+c.sendBytes(buf);
+}
+
+// Read the next server reply. If it is a protocol error then return it as an er ror.
+// Push events onto the event queue and stash away replies to requests in a map
+// indexed by the sequence number.
+func (c *Conn) readNextReply() os.Error {

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

Since this method reads both replies and events, perhaps a better name is just readNext(). 

+buf := make([]byte, 32);
+_, error := io.ReadFull(c.sock, buf);
+if error != nil {
+fmt.Fprintf(os.Stderr, "x protocol read error: %s\n", error);
+return error;
+}
+
+if buf[0] == 0 {

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

switch buf[0] { 

[tor, 2009/11/30 13:48:46]

Done. 

+detail := buf[1];
+cookie := get16(buf, 2);
+id := get32(buf, 4);
+minor := get16(buf, 8);
+major := buf[10];
+str := fmt.Sprintf("Bad%s (major=%d minor=%d cookie=%d id=0x%x)" ,
+errorNames[detail], major, minor, cookie, id);
+fmt.Fprintf(os.Stderr, "x protocol error: %s\n", str);
+return os.NewError(str);

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

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; 

[tor, 2009/11/30 13:48:46]

Done. 

+}
+
+if buf[0] == 1 {
+seq := get16(buf, 2);
+size := get32(buf, 4);
+if size > 0 {
+bigbuf := make([]byte, 32+size*4, 32+size*4);
+copy(bigbuf[0:32], buf);

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

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. 

[tor, 2009/11/30 13:48:46]

Done. 

+_, error = io.ReadFull(c.sock, bigbuf[32:len(bigbuf)]);
+if error != nil {
+fmt.Fprintf(os.Stderr, "x protocol read error: % s\n", error);
+return error;
+}
+c.replies[seq] = bigbuf;
+} else {
+c.replies[seq] = buf
+}
+}
+
+if buf[0] > 1 {
+c.events.queue(buf)
+}
+
+return nil;
+}
+
+// Looks for a reply in the map indexed by sequence number.
+// Will block while reading replies from the server until it is found.
+func (c *Conn) waitForReply(cookie uint16) ([]byte, os.Error) {
+for {
+reply, exists := c.replies[cookie];

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

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; } 

[tor, 2009/11/30 13:48:46]

I generally dislike putting more than one concept/instruction/whatsthename on each line, like the merged call and test idiom in the readNextReply statement. 

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

I'm sympathetic to that but it's still a common Go idiom and looks out of place here not to merge them. 

+if exists {
+c.replies[cookie] = reply, false;
+return reply, nil;
+}
+error := c.readNextReply();
+if error != nil {
+return nil, error
+}
+}
+panic("unreachable");
+}
+
+// Return the next event from the server.
+// Block until an event is available.
+func (c *Conn) WaitForEvent() (Event, os.Error) {
+for {
+reply := c.events.unqueue();
+if reply != nil {
+return parseEvent(reply), nil

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

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) 

[tor, 2009/11/30 13:48:46]

Done. 

+}
+error := c.readNextReply();
+if error != nil {
+return nil, error
+}
+}
+panic("unreachable");
+}
+
+// Return the next event from the server if one is available in the internal que ue.
+// This function will not read from the socket, so you must call WaitForEvent to receive new events.
+// Only use this function to empty the queue without blocking,
+// for instance when processing Expose events.
+func (c *Conn) PollForEvent() Event {
+reply := c.events.unqueue();
+if reply != nil {
+return parseEvent(reply)
+}
+return nil;
+}
+
+// Return the Screen info for the default screen (0 or the one given in the DISP LAY environment variable).

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

This mentions the DISPLAY environment variable but Dial doesn't seem to. s/DISPLAY environment variable/display string/? 

[tor, 2009/11/30 13:48:46]

Done. 

+func (c *Conn) DefaultScreen() *ScreenInfo { return &c.Setup.Roots[c.defaul tScreen] }
+
+// Connect to the X server given in the 'display' string.

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

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? 

[tor, 2009/11/30 13:48:46]

See my darcs repo for an example program: http://ghostscript.com/~tor/repos/xgb/main.go 

+func Dial(display string) (*Conn, os.Error) {
+var error os.Error;
+
+c := new(Conn);
+
+if display[0] == '/' {
+c.sock, error = net.Dial("unix", "", display);
+if error != nil {
+fmt.Printf("cannot connect: %v\n", error);
+return nil, error;
+}
+} else {
+parts := strings.Split(display, ":", 2);
+host := parts[0];
+port := 0;
+if len(parts) > 1 {
+parts = strings.Split(parts[1], ".", 2);
+port, _ = strconv.Atoi(parts[0]);
+if len(parts) > 1 {
+c.defaultScreen, _ = strconv.Atoi(parts[1])
+}
+}
+display = fmt.Sprintf("%s:%d", host, port+6000);
+c.sock, error = net.Dial("tcp", "", display);
+if error != nil {
+fmt.Printf("cannot connect: %v\n", error);
+return nil, error;
+}
+}
+
+var authName, authData []byte;

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

// TODO: Fill these in? 

[tor, 2009/11/30 13:48:46]

Indeed. It's one of the things I haven't done yet, reading the magic cookies from .Xauthority. 

+var buf []byte;

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

I would fold this line with the next one using the := short variable declaration. Specifically: buf := make(etcetera); 

[tor, 2009/11/30 13:48:46]

I put it separately because I reuse the variable for another buffer further down. 

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

You can still use := though. It's pretty unusual to see var declarations in a function body in Go. 

+
+buf = make([]byte, 12+pad(len(authName))+pad(len(authData)));
+buf[0] = 'l';
+buf[1] = 0;
+put16(buf, 2, 11);
+put16(buf, 4, 0);
+put16(buf, 6, uint16(len(authName)));
+put16(buf, 8, uint16(len(authData)));
+put16(buf, 10, 0);
+copy(buf[12:], authName);
+copy(buf[12+pad(len(authName)):], authData);
+_, error = c.sock.Write(buf);
+if error != nil {
+return nil, error
+}
+
+head := make([]byte, 8);
+_, error = io.ReadFull(c.sock, head[0:8]);
+if error != nil {
+return nil, error
+}
+code := head[0];
+reasonLen := head[1];
+major := get16(head, 2);
+minor := get16(head, 4);
+dataLen := get16(head, 6);
+
+if major != 11 || minor != 0 {
+return nil, os.NewError(fmt.Sprintf("x protocol version mismatch : %d.%d", major, minor))
+}
+
+buf = make([]byte, int(dataLen)*4+8, int(dataLen)*4+8);
+copy(buf, head);
+_, error = io.ReadFull(c.sock, buf[8:]);
+if error != nil {
+return nil, error
+}
+
+if code == 0 {
+reason := buf[8 : 8+reasonLen];
+return nil, os.NewError(fmt.Sprintf("x protocol authentication r efused: %s", string(reason)));
+}
+
+parseSetupInfo(buf, &c.Setup);
+
+if c.defaultScreen >= len(c.Setup.Roots) {
+c.defaultScreen = 0
+}
+
+c.nextId = c.Setup.ResourceIdBase;
+c.nextCookie = 1;
+c.replies = make(map[uint16][]byte);
+c.events = queue{make([][]byte, 100), 0, 0};
+return c, nil;
+}
+
+// Close the connection.
+func (c *Conn) Close() { c.sock.Close() }
+
+// Go doesn't have unions so we duplicate the data in all
+// three formats in ClientMessageData instead.
+type ClientMessageData struct /*union */ {

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

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. 

[tor, 2009/11/30 13:48:46]

Done. 

+Data8 [20]byte;
+Data16 [10]uint16;
+Data32 [5]uint32;
+}
+
+func parseClientMessageData(b []byte, v *ClientMessageData) int {
+copy(&v.Data8, b);
+for i := 0; i < 10; i++ {
+v.Data16[i] = get16(b, i*2)
+}
+for i := 0; i < 5; i++ {
+v.Data32[i] = get32(b, i*4)
+}
+return 20;
+}