hash: should use binary.BigEndian's AppendUint{32|64} and Uint{32|64} instead of handwritten helper functions

[apocelipes]

CL#66710 implements the BinaryMarshaler and BinaryUnmarshaler interfaces, which introduce handwritten helper functions like appendUint32/appendUint64/readUint32/readUint64.

For example, look at hash/crc64/crc64.go:

func appendUint64(b []byte, x uint64) []byte {
	a := [8]byte{
		byte(x >> 56),
		byte(x >> 48),
		byte(x >> 40),
		byte(x >> 32),
		byte(x >> 24),
		byte(x >> 16),
		byte(x >> 8),
		byte(x),
	}
	return append(b, a[:]...)
}

func readUint64(b []byte) uint64 {
	_ = b[7]
	return uint64(b[7]) | uint64(b[6])<<8 | uint64(b[5])<<16 | uint64(b[4])<<24 |
		uint64(b[3])<<32 | uint64(b[2])<<40 | uint64(b[1])<<48 | uint64(b[0])<<56
}

There are many of the same helper functions in other packages.

Since CL#386017, these functions become the public interface to the binary standard library.

Code using these helper functions can be easily rewritten, using crc64 as an example:

Before:

func (d *digest) MarshalBinary() ([]byte, error) {
	b := make([]byte, 0, marshaledSize)
	b = append(b, magic...)
	b = appendUint64(b, tableSum(d.tab))
	b = appendUint64(b, d.crc)
	return b, nil
}

func (d *digest) UnmarshalBinary(b []byte) error {
	if len(b) < len(magic) || string(b[:len(magic)]) != magic {
		return errors.New("hash/crc64: invalid hash state identifier")
	}
	if len(b) != marshaledSize {
		return errors.New("hash/crc64: invalid hash state size")
	}
	if tableSum(d.tab) != readUint64(b[4:]) {
		return errors.New("hash/crc64: tables do not match")
	}
	d.crc = readUint64(b[12:])
	return nil
}

After:

func (d *digest) MarshalBinary() ([]byte, error) {
	b := make([]byte, 0, marshaledSize)
	b = append(b, magic...)
	b = binary.BigEndian.AppendUint64(b, tableSum(d.tab))
	b = binary.BigEndian.AppendUint64(b, d.crc)
	return b, nil
}

func (d *digest) UnmarshalBinary(b []byte) error {
	if len(b) < len(magic) || string(b[:len(magic)]) != magic {
		return errors.New("hash/crc64: invalid hash state identifier")
	}
	if len(b) != marshaledSize {
		return errors.New("hash/crc64: invalid hash state size")
	}
	if tableSum(d.tab) != binary.BigEndian.Uint64(b[4:]) {
		return errors.New("hash/crc64: tables do not match")
	}
	d.crc = binary.BigEndian.Uint64(b[12:])
	return nil
}

There are these reasons below to support this refactoring:

• Reducing duplicate code and preventing code bloat
• Allows for a clearer indication of the data encoding method used (big-endian or little-endian)
• A cleaner implementation of AppendUint in the binary package (no need for temporary arrays)
• None of the modifications are public api's and will have no impact on language and standard library usage.

Packages that need to be refactored:

• hash/adler32
• hash/crc32
• hash/crc64
• hash/fnv

[randall77]

Taking out of the proposal process because there's no new API here.
Just do it, details can be argued over in CLs.

[apocelipes]

Taking out of the proposal process because there's no new API here. Just do it, details can be argued over in CLs.

Thanks. I will send a PR later.

[apocelipes]

There are some import cycles between hash and encoding/binary. So only we can do is to keep the helper functions as same as that in the binary package and let other code keep them it as.

[gopherbot]

Change https://go.dev/cl/537796 mentions this issue: hash: simplify binary operations