code review 5496059: sort: Reverse and some missing docs

sort: Reverse and some missing docs

[bradfitz, 2011-12-16 23:06:26]

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

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

[rsc, 2011-12-16 23:08:11]

it's hard to get excited about Reverse.
i like the doc comment for Sort.

[gri, 2011-12-16 23:11:03]

I don't mind the comments but I am not convinced about the Reverse.

http://codereview.appspot.com/5496059/diff/4001/src/pkg/sort/sort.go
File src/pkg/sort/sort.go (right):

http://codereview.appspot.com/5496059/diff/4001/src/pkg/sort/sort.go#newcode198
src/pkg/sort/sort.go:198: // IsSorted returns whether data is sorted.
IsSorted returns true if data is sorted. (?)

http://codereview.appspot.com/5496059/diff/4001/src/pkg/sort/sort.go#newcode257
src/pkg/sort/sort.go:257: // Reverse sorts data in reverse.
I'd rather leave this to the implementer of Less. As is, this is probably quite
a bit slower then "forward" Sort, which is probably not great.

[bradfitz, 2011-12-16 23:17:27]

On Fri, Dec 16, 2011 at 3:11 PM, <gri@golang.org> wrote:

> I don't mind the comments but I am not convinced about the Reverse.
>
>
>
http://codereview.appspot.com/**5496059/diff/4001/src/pkg/**sort/sort.go<http...
> File src/pkg/sort/sort.go (right):
>
> http://codereview.appspot.com/**5496059/diff/4001/src/pkg/**
>
sort/sort.go#newcode198<http://codereview.appspot.com/5496059/diff/4001/src/pkg/sort/sort.go#newcode198>
> src/pkg/sort/sort.go:198: // IsSorted returns whether data is sorted.
> IsSorted returns true if data is sorted. (?)
>

We've been gradually converging on "whether" for bools.  Not if, not iff,
not if & only if.  Or, so I thought.


> http://codereview.appspot.com/**5496059/diff/4001/src/pkg/**
>
sort/sort.go#newcode257<http://codereview.appspot.com/5496059/diff/4001/src/pkg/sort/sort.go#newcode257>
> src/pkg/sort/sort.go:257: // Reverse sorts data in reverse.
> I'd rather leave this to the implementer of Less. As is, this is
> probably quite a bit slower then "forward" Sort, which is probably not
> great.
>

I don't care about Reverse either, but I'm very confused by this statement.
 How would it be "quite a bit slower"?

[gri, 2011-12-16 23:23:39]

On Fri, Dec 16, 2011 at 3:17 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Fri, Dec 16, 2011 at 3:11 PM, <gri@golang.org> wrote:
>>
>> I don't mind the comments but I am not convinced about the Reverse.
>>
>>
>> http://codereview.appspot.com/5496059/diff/4001/src/pkg/sort/sort.go
>> File src/pkg/sort/sort.go (right):
>>
>>
>>
http://codereview.appspot.com/5496059/diff/4001/src/pkg/sort/sort.go#newcode198
>> src/pkg/sort/sort.go:198: // IsSorted returns whether data is sorted.
>> IsSorted returns true if data is sorted. (?)
>
>
> We've been gradually converging on "whether" for bools.  Not if, not iff,
> not if & only if.  Or, so I thought.

ok
>
>>
>>
>>
http://codereview.appspot.com/5496059/diff/4001/src/pkg/sort/sort.go#newcode257
>> src/pkg/sort/sort.go:257: // Reverse sorts data in reverse.
>> I'd rather leave this to the implementer of Less. As is, this is
>> probably quite a bit slower then "forward" Sort, which is probably not
>> great.
>
>
> I don't care about Reverse either, but I'm very confused by this statement.
>  How would it be "quite a bit slower"?

There's an extra indirection (call) for each call to Less. Probably not great.

[rsc, 2011-12-16 23:29:23]

"reports whether"

Reverse might be a nice wiki example.