It's annoying, but I think we need to define new types and functions. Specifically: 1. ...
11 years, 11 months ago
(2012-05-31 12:32:11 UTC)
#3
It's annoying, but I think we need to define new types and functions.
Specifically:
1. Keep the old Win32finddata as is, but add a comment:
// Win32finddata is an incorrect struct definition, preserved for
// backwards compatibility. Use Win32finddata1 and the
// FindFirstFile1 and FindNextFile1 functions instead.
type Win32finddata struct ...
2. Add the correct definition as type Win32finddata1.
3. Keep
syscall_windows.go:132: //sys FindFirstFile(name *uint16, data
*Win32finddata) (handle Handle, err error) [failretval==InvalidHandle]
= FindFirstFileW
syscall_windows.go:133: //sys FindNextFile(handle Handle, data
*Win32finddata) (err error) = FindNextFileW
4. Add two more lines for FindFirstFile1 and FindNextFile1, with the
new type for data.
Russ
Sorry for the 25th hour feedback. I just noticed this CL is being considered for ...
11 years, 10 months ago
(2012-06-07 02:57:13 UTC)
#7
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?
On 2012/06/07 04:01:29, rsc wrote: > You may be right. ... I think he is. ...
11 years, 10 months ago
(2012-06-07 04:17:34 UTC)
#9
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
On Thu, Jun 7, 2012 at 12:17 AM, <alex.brainman@gmail.com> wrote: > 2) If you take ...
11 years, 10 months ago
(2012-06-07 04:23:18 UTC)
#10
On Thu, Jun 7, 2012 at 12:17 AM, <alex.brainman@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
On 2012/06/07 04:23:18, rsc wrote: > > ... who wanted to be careful ... Isn't ...
11 years, 10 months ago
(2012-06-07 04:32:28 UTC)
#11
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
You're right - the old struct is useless and would have caused crashes (just mysterious ...
11 years, 10 months ago
(2012-06-07 04:44:21 UTC)
#12
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
On 2012/06/07 04:44:21, rsc wrote: > You're right - the old struct is useless and ...
11 years, 10 months ago
(2012-06-07 04:46:42 UTC)
#13
On 2012/06/07 04:44:21, rsc wrote:
> 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
>
Major cr...p, I know. I am all ashamed. :-)
Alex
On Wed, Jun 6, 2012 at 10:57 PM, <hcwfrichter@gmail.com> wrote: > Sorry for the 25th ...
11 years, 10 months ago
(2012-06-08 17:48:52 UTC)
#14
On Wed, Jun 6, 2012 at 10:57 PM, <hcwfrichter@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
Issue 6261053: code review 6261053: syscall: correct Win32finddata definition
(Closed)
Created 11 years, 11 months ago by brainman
Modified 11 years, 10 months ago
Reviewers: frichter
Base URL:
Comments: 0