Re: code review 6261053: syscall: correct Win32finddata definition (issue 6261053)

[alex.b...@gmail.com]

Hello golan...@googlegroups.com, r...@golang.org (cc:
golan...@googlegroups.com),

Please take another look.


http://codereview.appspot.com/6261053/

[r...@golang.org]

LGTM


http://codereview.appspot.com/6261053/

[alex.b...@gmail.com]

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

syscall: correct Win32finddata definition

Fixes issue 3685.

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


http://codereview.appspot.com/6261053/

[hcwfr...@gmail.com]

Sorry for the 25th hour feedback. I just noticed this CL is being
considered for Go1.0.2, and I think that the approach here was wrong.

I believe it violates the spirit of the "go1" promise "Code that
compiles in Go 1 should, with few exceptions, continue to compile and
run throughout the lifetime of that version, even as we issue updates
and bug fixes ..."

Code that used to compile and run (regardless of the potential for
memory smashing) will now compile and NO LONGER run. But the user won't
know until runtime.

Code that didn't run, still compiles and still won't work properly
without a source code change that the user has no way to know is
required until he gets a new runtime error.

Code that does not yet exist could, in theory, try to use the still
existing, but now "disabled" API (it will still show up in the docs
won't it?), and then have to be changed again when it fails at runtime.

None of these seems intuitive or programmer friendly.


Alex's original patch set 3 would appear to have been the proper choice
for any of these cases; people will just recompile existing code and it
would "just work". Fix go1.txt to make the API checker happy with the
actual Windows data structure.

This is not an API "change", it would just mean bringing go1.txt in line
with reality for an OS specific data structure.

Or perhaps I am missing some bigger issue?




http://codereview.appspot.com/6261053/

[Russ Cox]

You may be right. There is also CL 6244051. I'm going to talk to
Andrew and others about it next week.

Russ

[alex.b...@gmail.com]

On 2012/06/07 04:01:29, rsc wrote:
> You may be right. ...

I think he is. This case is broken any way you look at it:

1) If you take my patch #3, then some old programs might not compile;

2) If you take submitted change, then these programs will compile, but
will certainly crash during execution.

So, it is a question of one out of 2 evils. I prefer 1).

Alex

http://codereview.appspot.com/6261053/

[Russ Cox]

On Thu, Jun 7, 2012 at 12:17 AM, <alex.b...@gmail.com> wrote:
> 2) If you take submitted change, then these programs will compile, but
> will certainly crash during execution.

I would have preferred your change to leave the old behavior, so that
someone who wanted to be careful could still write code that worked
with the original Go 1 structures (by interpreting the byte arrays
themselves). That might still be worth doing.

Russ

[alex.b...@gmail.com]

On 2012/06/07 04:23:18, rsc wrote:

> ... who wanted to be careful ...

Isn't Go supposed to hold our hand here? It is very hard to think about
bugs in your program, when you know that there are memory corruption
issues in it.

> ... That might still be worth doing.

Fair enough. But I still do not see how. Unless, you just want to honor
"you program will compile" promise.

Alex

http://codereview.appspot.com/6261053/

[Russ Cox]

You're right - the old struct is useless and would have caused crashes
(just mysterious ones). I misremembered the change. I thought it was

[N]byte
[M]byte

->

[N+1]byte
[M-1]byte

but it is ->

[N+1]byte
[M+1]byte

Russ

[alex.b...@gmail.com]

Major cr...p, I know. I am all ashamed. :-)

Alex

http://codereview.appspot.com/6261053/

[Russ Cox]

On Wed, Jun 6, 2012 at 10:57 PM, <hcwfr...@gmail.com> wrote:
> Sorry for the 25th hour feedback.  I just noticed this CL is being
> considered for Go1.0.2, and I think that the approach here was wrong.

It's certainly true that we shouldn't do this for Go 1.0.2. Thank you
for reminding us.

I have sent out CL 6298063, which hides the new system calls and
structures and implements the old ones using them. This avoids any
visible API change and still gives access to the 8.3 name. For Go 1.1,
we might do something to avoid the overhead of the translation.

Russ