code review 4315050: crypto/des: cleanups

crypto/des: cleanups

[agl1, 2011-04-05 18:22:53]

Please be nit-picky, might as well do this in one.

[rsc, 2011-04-05 19:16:11]

LGTM

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/block.go
File src/pkg/crypto/des/block.go (right):

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:61: block |= bit <<
uint64((len(permutation)-1)-position)
s/uint64/uint/

The shift just needs an unsigned right arg, it doesn't
have to be the same type as the left.  On a 32-bit machine
the uint shift will be a little easier to implement.

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:73: left := (last << (4 + uint32(ksRotations[i])))
>> 4
can drop uint32 conversion

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/const.go
File src/pkg/crypto/des/const.go (right):

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/const.go#n...
src/pkg/crypto/des/const.go:82: [16]uint8{14, 4, 13, 1, 2, 15, 11, 8, 3, 10, 6,
12, 5, 9, 0, 7},
Can drop all these [16]uint8

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/des_test.go
File src/pkg/crypto/des/des_test.go (right):

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1277: for j, v := range result {
Please replace all these loops with

if !bytes.Equal(x, y) {
    t.Errorf("#%d: result=%x want %x", i, x, y)
}

[agl1, 2011-04-05 19:40:47]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[agl1, 2011-04-05 19:40:56]

*** Submitted as http://code.google.com/p/go/source/detail?r=b11e62f75948 ***

crypto/des: cleanups

R=rsc
CC=golang-dev
http://codereview.appspot.com/4315050

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/block.go
File src/pkg/crypto/des/block.go (right):

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:61: block |= bit <<
uint64((len(permutation)-1)-position)
On 2011/04/05 19:16:11, rsc wrote:
> s/uint64/uint/
> 
> The shift just needs an unsigned right arg, it doesn't
> have to be the same type as the left.  On a 32-bit machine
> the uint shift will be a little easier to implement.

Done.

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:73: left := (last << (4 + uint32(ksRotations[i])))
>> 4
On 2011/04/05 19:16:11, rsc wrote:
> can drop uint32 conversion

Done.

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/const.go
File src/pkg/crypto/des/const.go (right):

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/const.go#n...
src/pkg/crypto/des/const.go:82: [16]uint8{14, 4, 13, 1, 2, 15, 11, 8, 3, 10, 6,
12, 5, 9, 0, 7},
On 2011/04/05 19:16:11, rsc wrote:
> Can drop all these [16]uint8

Done.

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/des_test.go
File src/pkg/crypto/des/des_test.go (right):

http://codereview.appspot.com/4315050/diff/2001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1277: for j, v := range result {
On 2011/04/05 19:16:11, rsc wrote:
> Please replace all these loops with
> 
> if !bytes.Equal(x, y) {
>     t.Errorf("#%d: result=%x want %x", i, x, y)
> }

Done.