code review 5855044: sort: document two undocumented functions

sort: document two undocumented functions

They looked out of place in godoc.
Includes documenting sort stability.

Fixes issue 3356

[bradfitz, 2012-03-20 18:09:59]

Hello golang-dev@googlegroups.com,

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

[gri, 2012-03-20 18:18:30]

LGTM

Please add to the CL description:

Fixes issue 3356.

- gri

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

http://codereview.appspot.com/5855044/diff/3002/src/pkg/sort/sort.go#newcode199
src/pkg/sort/sort.go:199: // IsSorted returns whether data is sorted.
s/returns/reports/

[Stefan Nilsson, 2012-03-20 18:19:54]

It would be nice to add that Sort makes at most O(n log n) calls to the
Interface methods.

[bradfitz, 2012-03-20 18:26:53]

On Tue, Mar 20, 2012 at 11:18 AM, <gri@golang.org> wrote:

> LGTM
>
> Please add to the CL description:
>
> Fixes issue 3356.
>

ah, didn't know there was an issue. Done.



> http://codereview.appspot.com/**5855044/diff/3002/src/pkg/**sort/sort.go
> File src/pkg/sort/sort.go (right):
>
> http://codereview.appspot.com/**5855044/diff/3002/src/pkg/**
> sort/sort.go#newcode199
> src/pkg/sort/sort.go:199: // IsSorted returns whether data is sorted.
> s/returns/reports/


is that a thing?

bradfitz@bradfitzlap:~/go/src/pkg/sort$ grep -r "returns whether"
$GOROOT/src | wc -l
76
bradfitz@bradfitzlap:~/go/src/pkg/sort$ grep -r "reports whether"
$GOROOT/src | wc -l
103
bradfitz@bradfitzlap:~/go/src/pkg/sort$ grep -r "reports whether"
$GOROOT/src/pkg/sort | wc -l
0
bradfitz@bradfitzlap:~/go/src/pkg/sort$ grep -r "returns whether"
$GOROOT/src/pkg/sort | wc -l
2

Should I update the existing "returns whether" that's already in sort.go?
 At least how I have it now is locally consistent.

[gri, 2012-03-20 18:27:59]

We generally don't have performance guarantees in the docs, I think.
Also, I am not sure this is actually true now for the current
implementation. I am in favor of not specifying it.

On Tue, Mar 20, 2012 at 11:19 AM,  <trolleriprofessorn@gmail.com> wrote:
> It would be nice to add that Sort makes at most O(n log n) calls to the
> Interface methods.
>
> http://codereview.appspot.com/5855044/

[Stefan Nilsson, 2012-03-20 18:32:13]

It is true (after the heapsort CL). In this case I would say it's important to
document this fact. Most languages offer system sorts with quadratic worst-case
time. Also, there are DoS attacks that use this weakness of many quicksort
implementations.

On 2012/03/20 18:27:59, gri wrote:
> We generally don't have performance guarantees in the docs, I think.
> Also, I am not sure this is actually true now for the current
> implementation. I am in favor of not specifying it.
> 
> On Tue, Mar 20, 2012 at 11:19 AM,  <mailto:trolleriprofessorn@gmail.com>
wrote:
> > It would be nice to add that Sort makes at most O(n log n) calls to the
> > Interface methods.
> >
> > http://codereview.appspot.com/5855044/

[bradfitz, 2012-03-20 18:40:18]

Documenting all the ways in which we can't be attacked seems incomplete and
overly hopeful, not to mention verbose.

On Tue, Mar 20, 2012 at 11:32 AM, <trolleriprofessorn@gmail.com> wrote:

> It is true (after the heapsort CL). In this case I would say it's
> important to document this fact. Most languages offer system sorts with
> quadratic worst-case time. Also, there are DoS attacks that use this
> weakness of many quicksort implementations.
>
>
> On 2012/03/20 18:27:59, gri wrote:
>
>> We generally don't have performance guarantees in the docs, I think.
>> Also, I am not sure this is actually true now for the current
>> implementation. I am in favor of not specifying it.
>>
>
>  On Tue, Mar 20, 2012 at 11:19 AM,
>>
> <mailto:trolleriprofessorn@**gmail.com <trolleriprofessorn@gmail.com>>
> wrote:
>
>> > It would be nice to add that Sort makes at most O(n log n) calls to
>>
> the
>
>> > Interface methods.
>> >
>> >
http://codereview.appspot.com/**5855044/<http://codereview.appspot.com/5855044/>
>>
>
>
>
>
http://codereview.appspot.com/**5855044/<http://codereview.appspot.com/5855044/>
>

[bradfitz, 2012-03-20 18:40:45]

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

sort: document two undocumented functions

They looked out of place in godoc.
Includes documenting sort stability.

Fixes issue 3356

R=golang-dev, gri, trolleriprofessorn
CC=golang-dev
http://codereview.appspot.com/5855044

[Stefan Nilsson, 2012-03-20 19:38:13]

Documenting DoS attacks certainly would be a very bad idea. I only suggested
adding information about the running time. This clearly is important
information to have when you call a library function. The origial CL 4591051
that removed the quadratic running time fixed a long standing issue 467.
So yes, people are interested in knowing this.


On 2012/03/20 18:40:18, bradfitz wrote:
> Documenting all the ways in which we can't be attacked seems incomplete and
> overly hopeful, not to mention verbose.

[bradfitz, 2012-03-20 19:40:37]

Feel free to pursue it in a subsequent CL.  My immediate itch was just
fixing the lack of documentation.


On Tue, Mar 20, 2012 at 12:38 PM, <trolleriprofessorn@gmail.com> wrote:

> Documenting DoS attacks certainly would be a very bad idea. I only
> suggested
> adding information about the running time. This clearly is important
> information to have when you call a library function. The origial CL
> 4591051
> that removed the quadratic running time fixed a long standing issue 467.
> So yes, people are interested in knowing this.
>
>
>
> On 2012/03/20 18:40:18, bradfitz wrote:
>
>> Documenting all the ways in which we can't be attacked seems
>>
> incomplete and
>
>> overly hopeful, not to mention verbose.
>>
>
>
http://codereview.appspot.com/**5855044/<http://codereview.appspot.com/5855044/>
>

[Stefan Nilsson, 2012-03-20 20:02:12]

Sure, I'll do that. It wasn't my intention to hijack this CL,
it just seemed like a natural addition to the doc to me.

On 2012/03/20 19:40:37, bradfitz wrote:
> Feel free to pursue it in a subsequent CL.  My immediate itch was just
> fixing the lack of documentation.
> 
> 
> On Tue, Mar 20, 2012 at 12:38 PM, <mailto:trolleriprofessorn@gmail.com> wrote:
> 
> > Documenting DoS attacks certainly would be a very bad idea. I only
> > suggested
> > adding information about the running time. This clearly is important
> > information to have when you call a library function. The origial CL
> > 4591051
> > that removed the quadratic running time fixed a long standing issue 467.
> > So yes, people are interested in knowing this.
> >
> >
> >
> > On 2012/03/20 18:40:18, bradfitz wrote:
> >
> >> Documenting all the ways in which we can't be attacked seems
> >>
> > incomplete and
> >
> >> overly hopeful, not to mention verbose.
> >>
> >
> >
>
http://codereview.appspot.com/**5855044/%3Chttp://codereview.appspot.com/5855...>
> >