code review 9157044: syscall: fix prototype of Fchflags (API change)

syscall: fix prototype of Fchflags (API change)
API change, but the old API is obviously wrong.

[minux1, 2013-05-03 09:10:20]

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

I'd like you to review this change to
https://code.google.com/p/go/

[minux1, 2013-05-03 09:13:11]

I'd like to get this in before Go 1.1, because:
1. it fixes a broken API that is otherwise useless,
2. it's fairly isolated change,
3. given our policy of no API change in minor releases, if we don't fix it
in 1.1, it will have to wait for 1.2.

comments?

[iant, 2013-05-03 13:15:32]

LGTM

Please also add a note to doc/go1.1.html.

Wait for r.

[minux1, 2013-05-03 14:35:15]

On 2013/05/03 13:15:32, iant wrote:
> Please also add a note to doc/go1.1.html.
Done. PTAL.

[r, 2013-05-03 14:42:22]

https://codereview.appspot.com/9157044/diff/13001/doc/go1.1.html
File doc/go1.1.html (right):

https://codereview.appspot.com/9157044/diff/13001/doc/go1.1.html#newcode1026
doc/go1.1.html:1026: The <a href="/pkg/syscall/"><code>syscall</code></a>
package's <a href="/pkg/syscall/#Fchflags"><code>Fchflags</code></a> function on
various BSDs (including Darwin) has changed parameter type: it now takes an int
as the first parameter, instead of a string, which is clearly a mistake in Go
1.0. Since this API change fixes a bug, it is permitted by the Go 1
compatibility rules.
fold these long  lines. start sentences on a new line. see the rest of the
document for guidance.

s/parameter type/signature/
s/: i/.\nI/
s/, which.*1.0//

[minux1, 2013-05-03 15:00:26]

PTAL.

https://codereview.appspot.com/9157044/diff/13001/doc/go1.1.html
File doc/go1.1.html (right):

https://codereview.appspot.com/9157044/diff/13001/doc/go1.1.html#newcode1026
doc/go1.1.html:1026: The <a href="/pkg/syscall/"><code>syscall</code></a>
package's <a href="/pkg/syscall/#Fchflags"><code>Fchflags</code></a> function on
various BSDs (including Darwin) has changed parameter type: it now takes an int
as the first parameter, instead of a string, which is clearly a mistake in Go
1.0. Since this API change fixes a bug, it is permitted by the Go 1
compatibility rules.
On 2013/05/03 14:42:23, r wrote:
> fold these long  lines. start sentences on a new line. see the rest of the
> document for guidance.
> 
> s/parameter type/signature/
> s/: i/.\nI/
> s/, which.*1.0//
Done.

[r, 2013-05-03 15:24:14]

LGTM

https://codereview.appspot.com/9157044/diff/21001/doc/go1.1.html
File doc/go1.1.html (right):

https://codereview.appspot.com/9157044/diff/21001/doc/go1.1.html#newcode1029
doc/go1.1.html:1029: It now takes an int as the first parameter, instead of a
string.
s/,//

[rsc, 2013-05-03 16:01:48]

I thought we were committed to keeping the old APIs even in the case of
bugs like this. In this case I would introduce an Fchflags2 or something
like that. But if someone can justify it using the compatibility doc, fine.

FWIW, the thing about "since this fixes a bug the API change is allowed" is
now in the doc twice, and I don't believe we ever said that was the case.
Anything can be justified by "this fixes a bug". But I won't argue more
about it.

Russ

[r, 2013-05-03 16:58:48]

I've been taking the "Bugs" clause to cover this case, but on
rereading it's not so clear. Still, I believe it's warranted since the
existing API is broken enough to be useless and I don't believe it's a
disservice to repair the mistake: no existing code can use the old
function profitably.

-rob

[minux1, 2013-05-03 17:06:26]

On Sat, May 4, 2013 at 12:01 AM, Russ Cox <rsc@golang.org> wrote:

> I thought we were committed to keeping the old APIs even in the case of
> bugs like this. In this case I would introduce an Fchflags2 or something
> like that. But if someone can justify it using the compatibility doc, fine.
>
I think the guarding condition is: don't break existing Go 1 code. The old
API is not at all usable
and there isn't workaround available to use that API.

>
> FWIW, the thing about "since this fixes a bug the API change is allowed"
> is now in the doc twice, and I don't believe we ever said that was the
> case. Anything can be justified by "this fixes a bug". But I won't argue
> more about it.
>
right, i copied the clause from the net.ListenUnixgram case.
do you want me to delete this clause or rephrase it?

[iant, 2013-05-03 17:56:54]

On Fri, May 3, 2013 at 9:01 AM, Russ Cox <rsc@golang.org> wrote:
> I thought we were committed to keeping the old APIs even in the case of bugs
> like this. In this case I would introduce an Fchflags2 or something like
> that. But if someone can justify it using the compatibility doc, fine.
>
> FWIW, the thing about "since this fixes a bug the API change is allowed" is
> now in the doc twice, and I don't believe we ever said that was the case.
> Anything can be justified by "this fixes a bug". But I won't argue more
> about it.

This is a case where the old code was completely non-functional.  Any
program that called it was broken.  There isn't any wiggle room about
different behaviour on different systems or anything.  So I think this
is a reasonable place to break the API.  There is a "Bugs" section in
the compatibility doc, and I think it covers this sort of case.

Ian

[r, 2013-05-06 20:12:42]

Please check this in.

-rob

[minux1, 2013-05-06 21:20:19]

*** Submitted as https://code.google.com/p/go/source/detail?r=34b8901146ac ***

syscall: fix prototype of Fchflags (API change)
API change, but the old API is obviously wrong.

R=golang-dev, iant, r, rsc
CC=golang-dev
https://codereview.appspot.com/9157044