godrv/driver.go: ineffective assignment

[kevinburke]

This function:

func (c conn) Close() (err error) {
	err = c.my.Close()
	c.my = nil
	if err != nil {
		err = errFilter(err)
	}
	return
}

I believe c.my = nil doesn't do anything, since conn is not a pointer - it gets copied when you call Close(). You can fix this by running:

func (c *conn) Close() (err error) {
	err = c.my.Close()
	c.my = nil
	if err != nil {
		err = errFilter(err)
	}
	return
}

I found this by running go-staticcheck on the codebase: https://github.com/dominikh/go-staticcheck

[kevinburke]

Looks like the same problem exists in two other places

// Commit a transaction
func (tr Transaction) Commit() error {
	_, err := tr.Start("COMMIT")
	tr.Conn = nil // Invalidate this transaction
	return err
}

// Rollback a transaction
func (tr Transaction) Rollback() error {
	_, err := tr.Start("ROLLBACK")
	tr.Conn = nil // Invalidate this transaction
	return err
}

In each case tr.Conn is still valid on the Transaction after the function call.

Happy to submit a PR to fix.

[ziutek]

Thanks. I have fixed first case.

Second is problematic, because Begin returns Transaction (not *Transaction).

I have written this library a long time ago, when I was little experience with Go, especially with many subtle things like this. Avoid double reference looked reasonable but now I prefer return pointer, even if struct contains only on pointer field. Much benefit of avoid double reference disappeared when Go started (Go 1.5?) to always store pointer to value in interface, even if value is of pointer size.

Can't change Begin without break many applications. Maybe invalidate Transaction.Conn can be workaround...