Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: FileInfo.Sys should return underlying data source on Windows #9611

Closed
perillo opened this issue Jan 16, 2015 · 5 comments
Closed

os: FileInfo.Sys should return underlying data source on Windows #9611

perillo opened this issue Jan 16, 2015 · 5 comments

Comments

@perillo
Copy link
Contributor

perillo commented Jan 16, 2015

The documentation of the Sys method for the FileInfo interface says:
"underlying data source (can return nil)"

However the Windows implementation does not do this.

The underlying data source for the FileInfo implementation in the Stat function is
syscall.ByHandleFileInformation, however the Sys method returns syscall.Win32FileAttributeData.

The underlying data source for the FileInfo implementation in the Readdir method is syscall.Win32finddata, however the Sys method returns syscall.Win32FleAttributeData.

The advantage of the current implementation is that Sys returns the same dynamic type, as it is done on Unix systems. However the disadvantage is that some valuable information (VolumeInformation and FineIndex from the Stat call) is thrown away. The current implementation also has to allocate additional space.

Since the interface documentation seems to allow it, I propose that the Sys method returns the actual underlying data used by the implementation, ad discussed above.

The change does not break the go 1 compatibility promise.

@mikioh mikioh changed the title FileInfo.Sys should return underlying data source on Windows os: FileInfo.Sys should return underlying data source on Windows Jan 16, 2015
@alexbrainman
Copy link
Member

@ianlancetaylor please assign it to me, if you want me to try and implement it. Tank you.

Alex

@ianlancetaylor
Copy link
Contributor

I don't have an opinion on whether we should do this or not. I'll assign it to you so that you can decide.

@minux
Copy link
Member

minux commented Jan 16, 2015

I still think this is a API change, although a less obvious one.

@alexbrainman
Copy link
Member

I don't think we should be making this change. We don't want to break people's code. Closing this as "unfortunate".

https://groups.google.com/forum/#!topic/golang-dev/Kky-9-TrcWY for full discussion.

Alex

@perillo
Copy link
Contributor Author

perillo commented Jan 18, 2015

On Sun, Jan 18, 2015 at 6:43 AM, alexbrainman notifications@github.com
wrote:

I don't think we should be making this change. We don't want to break
people's code. Closing this as "unfortunate".

It's unfortunate, since this is a private API and the go fix tool should be
able to upgrade the old code.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants