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
Assignees

Comments

@perillo
Copy link

@perillo 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

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 16, 2015

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

Alex

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 16, 2015

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

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

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

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 18, 2015

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

This comment has been minimized.

Copy link
Author

@perillo 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.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.