code review 5448079: os: turn FileStat.Sys into a method on FileInfo

os: turn FileStat.Sys into a method on FileInfo

This reduces the overhead necessary to work with OS-specific
file details, hides the implementation of FileStat, and
preserves the implementation-specific nature of Sys.

Expressions such as:

  stat.(*os.FileInfo).Sys.(*syscall.Stat_t).Uid
  fi1.(*os.FileStat).SameFile(fi2.(*os.FileStat))

Are now spelled as::

  stat.Sys().(*syscall.Stat_t).Uid
  os.SameFile(fi1, fi2)

[niemeyer, 2011-12-01 21:05:40]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-12-01 21:06:55]

This fails on systems without a Stat_t, sorry.

[rsc, 2011-12-01 21:09:19]

On Thu, Dec 1, 2011 at 16:06, Russ Cox <rsc@golang.org> wrote:
> This fails on systems without a Stat_t, sorry.

Also, and this might be worse, it means that you
can get at syscall-specific data without importing
syscall, which means you don't even know that
you're doing it.

I'd like to sit on this for a few days and think a bit
more about how to make this easier.  I don't mind
it being easier but I like very much the fact that
Unix-specific programs are marked as such, and
I would prefer to see a way to make things
easier that preserves that property.

Russ

[niemeyer, 2011-12-01 21:20:27]

> This fails on systems without a Stat_t, sorry.

These may easily define it as struct{}, or interface{} for that matter.

(...)
> I'd like to sit on this for a few days and think a bit
> more about how to make this easier.  I don't mind
> it being easier but I like very much the fact that
> Unix-specific programs are marked as such, and
> I would prefer to see a way to make things
> easier that preserves that property.

I appreciate that people know they are touching system specific data
as well, and I'll be happy to see an approach that preserves that
property. The problem I'm trying to address is that while migrating
actual code to the new approach, it's looking overly verbose to do
things that are often necessary when people are interacting with
common O.S. specific file details.

Let me try a slightly different approach that you may like better.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rsc, 2011-12-01 21:26:23]

On Thu, Dec 1, 2011 at 16:20, Gustavo Niemeyer <n13m3y3r@gmail.com>
wrote:>> This fails on systems without a Stat_t, sorry.>> These may
easily define it as struct{}, or interface{} for that matter.
I started down that path.  struct{} is useless, becauseyou do want to
be able to store some associated dataon those systems, and interface{}
is surprising, becausethen *Stat_t is *interface{}.  And on Windows
there is noteven a single struct type: depending on whether the info
came from Readdir or Stat, it has different types.
> I appreciate that people know they are touching system specific data
> as well, and I'll be happy to see an approach that preserves that
> property. The problem I'm trying to address is that while migrating
> actual code to the new approach, it's looking overly verbose to do
> things that are often necessary when people are interacting with
> common O.S. specific file details.

I understand and appreciate that.  However, it is worth pointing
out that you don't have to migrate actual code to the new
approach today.  This is a weekly snapshot, not a release.
I'd be happy to discuss proposals about ways to make this
better, but let's do that (discuss) instead of sending CLs.

Russ

[rsc, 2011-12-01 21:26:48]

On Thu, Dec 1, 2011 at 16:26, Russ Cox <rsc@golang.org> wrote:
> On Thu, Dec 1, 2011 at 16:20, Gustavo Niemeyer <n13m3y3r@gmail.com>
> wrote:>> This fails on systems without a Stat_t, sorry.>> These may
> easily define it as struct{}, or interface{} for that matter.
> I started down that path.  struct{} is useless, becauseyou do want to
> be able to store some associated dataon those systems, and interface{}
> is surprising, becausethen *Stat_t is *interface{}.  And on Windows
> there is noteven a single struct type: depending on whether the info
> came from Readdir or Stat, it has different types.

I apologize on Chrome's behalf.

[gustavo_niemeyer.net, 2011-12-01 21:43:01]

What about introducing in syscall:

   func Stat(s Iface) *Stat_t

where Iface is defined as:

    type Iface interface {
        func SysStat() interface{}
    }

and replacing the Sys field by SysStat() in os.FileInfo?
On Dec 1, 2011 7:26 PM, "Russ Cox" <rsc@golang.org> wrote:

[niemeyer, 2011-12-01 22:32:47]

> I understand and appreciate that.  However, it is worth pointing
> out that you don't have to migrate actual code to the new
> approach today.  This is a weekly snapshot, not a release.
> I'd be happy to discuss proposals about ways to make this
> better, but let's do that (discuss) instead of sending CLs.

Also, FWIW, one of the points of migrating now is precisely so that I
can get a feeling for the API so that we can have that conversation
while it's still comfortable for everybody to improve the interface.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[gustavo_niemeyer.net, 2011-12-01 22:44:44]

> What about introducing in syscall:

I've experimented a bit with this idea and, to be honest, even moving
the system-specific value to the Sys() function onto the FileInfo
interface itself would already feel much better:

    stat.Sys().(*syscall.Stat_t)

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[niemeyer, 2011-12-01 23:45:21]

>     stat.Sys().(*syscall.Stat_t)

I have updated the CL to implement this approach, in case someone is interested
in observing how it would look like.

[cw, 2011-12-02 18:50:56]

For posix-like systems might we not just have something like

  pstat := os.PosixStat(someFileInfo)

then pstat.Dev could be used directly similar to what was possible before?

[rsc, 2012-01-31 16:27:54]

I've been thinking about this for a few months,
and the zip change pushed me over the edge.
Please refresh this CL against the latest tree and
we can push it forward.  The implementation of
Sys for archive/zip's headFileInfo should return
the *FileHeader.

Thanks.

http://codereview.appspot.com/5448079/diff/5004/src/pkg/os/types.go
File src/pkg/os/types.go (right):

http://codereview.appspot.com/5448079/diff/5004/src/pkg/os/types.go#newcode22
src/pkg/os/types.go:22: Sys() interface{}   // O.S. specific details
// underlying data source

Might not have anything to do with operating systems
(it could be a *zip.Header, for example).

http://codereview.appspot.com/5448079/diff/5004/src/pkg/os/types.go#newcode92
src/pkg/os/types.go:92: type FileStat struct {
s/FileStat/fileStat/

[bradfitz, 2012-01-31 16:32:18]

Could you elaborate why you like this?  It feels like a regression to me.

If we care what an os.FileInfo is, we can check the concrete type of the
fileinfo interface as well as we can check the concrete type of
fileinfo.Sys().  What does this buy us?  It just expands this interface
more widely.  I already think IsDir() is pretty gross.


On Tue, Jan 31, 2012 at 8:27 AM, <rsc@golang.org> wrote:

> I've been thinking about this for a few months,
> and the zip change pushed me over the edge.
> Please refresh this CL against the latest tree and
> we can push it forward.  The implementation of
> Sys for archive/zip's headFileInfo should return
> the *FileHeader.
>
> Thanks.
>
>
>
>
http://codereview.appspot.com/**5448079/diff/5004/src/pkg/os/**types.go<http:...
> File src/pkg/os/types.go (right):
>
> http://codereview.appspot.com/**5448079/diff/5004/src/pkg/os/**
>
types.go#newcode22<http://codereview.appspot.com/5448079/diff/5004/src/pkg/os/types.go#newcode22>
> src/pkg/os/types.go:22: Sys() interface{}   // O.S. specific details
> // underlying data source
>
> Might not have anything to do with operating systems
> (it could be a *zip.Header, for example).
>
> http://codereview.appspot.com/**5448079/diff/5004/src/pkg/os/**
>
types.go#newcode92<http://codereview.appspot.com/5448079/diff/5004/src/pkg/os/types.go#newcode92>
> src/pkg/os/types.go:92: type FileStat struct {
> s/FileStat/fileStat/
>
>
http://codereview.appspot.com/**5448079/<http://codereview.appspot.com/5448079/>
>

[rsc, 2012-01-31 16:45:59]

On Tue, Jan 31, 2012 at 11:32, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> If we care what an os.FileInfo is, we can check the concrete type of the
> fileinfo interface as well as we can check the concrete type of
> fileinfo.Sys().  What does this buy us?  It just expands this interface more
> widely.  I already think IsDir() is pretty gross.

Suppose I am holding something like

var fi os.FileInfo = obtained-from-archive/zip

Fill in the code here:

var h *zip.FileHeader = ___data_behind_fi____

Without Sys (or some other name with the same definition),
this is impossible with the current archive/zip.  We could
of course export the type headerFileInfo, but that's yet another
type to show, and a more complex API.  This is going to keep
coming up as we need to provide implementations of os.FileInfo
based on structs with field names that conflict with the FileInfo
method names.  We can create, export, and document a new
wrapper type (like os.FileStat and zip.headerFileInfo) every time
this happens, or we can define the Sys method.

Russ

[bradfitz, 2012-01-31 16:49:02]

On Tue, Jan 31, 2012 at 8:45 AM, Russ Cox <rsc@golang.org> wrote:

> On Tue, Jan 31, 2012 at 11:32, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > If we care what an os.FileInfo is, we can check the concrete type of the
> > fileinfo interface as well as we can check the concrete type of
> > fileinfo.Sys().  What does this buy us?  It just expands this interface
> more
> > widely.  I already think IsDir() is pretty gross.
>
> Suppose I am holding something like
>
> var fi os.FileInfo = obtained-from-archive/zip
>
> Fill in the code here:
>
> var h *zip.FileHeader = ___data_behind_fi____
>
> Without Sys (or some other name with the same definition),
> this is impossible with the current archive/zip.  We could
> of course export the type headerFileInfo, but that's yet another
> type to show, and a more complex API.  This is going to keep
> coming up as we need to provide implementations of os.FileInfo
> based on structs with field names that conflict with the FileInfo
> method names.  We can create, export, and document a new
> wrapper type (like os.FileStat and zip.headerFileInfo) every time
> this happens, or we can define the Sys method.
>

I would prefer we do what we did to archive/zip each time (which I don't
think will be often enough), rather than expose a Sys().

If I give somebody an os.FileInfo, I want to be confident they'll accept
it, and not judge it based on its Sys() (or lack thereof).

[rsc, 2012-01-31 16:52:05]

On Tue, Jan 31, 2012 at 11:49, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I would prefer we do what we did to archive/zip each time (which I don't
> think will be often enough), rather than expose a Sys().

You would prefer to export and document zip.headerFileInfo as
zip.HeaderFileInfo?
That's the alternative, not doing nothing.

> If I give somebody an os.FileInfo, I want to be confident they'll accept it,
> and not judge it based on its Sys() (or lack thereof).

People can already judge os.FileInfo implementations with an interface check.
This does not affect that ability.

Russ

[gustavo_niemeyer.net, 2012-01-31 17:16:30]

On Tue, Jan 31, 2012 at 14:27,  <rsc@golang.org> wrote:
> I've been thinking about this for a few months,
> and the zip change pushed me over the edge.
> Please refresh this CL against the latest tree and
> we can push it forward.  The implementation of
> Sys for archive/zip's headFileInfo should return
> the *FileHeader.

Will do.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[niemeyer, 2012-02-01 01:28:51]

Hello rsc@golang.org, gustavo@niemeyer.net, cw@f00f.org, bradfitz@golang.org
(cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-02-02 18:36:02]

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go
File src/pkg/os/types.go (right):

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go#newcode115
src/pkg/os/types.go:115: // the decision may be based on the path names.
add
// SameFile only applies to results returned by this package's Stat.
// It returns false in other cases.

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go#newcode120
src/pkg/os/types.go:120: panic("SameFile: unsupported FileInfo")
return false

[niemeyer, 2012-02-02 19:29:46]

Hello cw@f00f.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[niemeyer, 2012-02-02 19:29:52]

PTAL

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go
File src/pkg/os/types.go (right):

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go#newcode115
src/pkg/os/types.go:115: // the decision may be based on the path names.
On 2012/02/02 18:36:02, rsc wrote:
> add
> // SameFile only applies to results returned by this package's Stat.
> // It returns false in other cases.

Done.

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go#newcode120
src/pkg/os/types.go:120: panic("SameFile: unsupported FileInfo")
On 2012/02/02 18:36:02, rsc wrote:
> return false

Done.

[bradfitz, 2012-02-02 19:33:19]

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go
File src/pkg/os/types.go (right):

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go#newcode22
src/pkg/os/types.go:22: Sys() interface{}   // underlying data source
can this say something like "optional" or ", or nil"?

[cw, 2012-02-02 19:40:14]

http://codereview.appspot.com/5448079/diff/23001/src/cmd/godoc/zip.go
File src/cmd/godoc/zip.go (right):

http://codereview.appspot.com/5448079/diff/23001/src/cmd/godoc/zip.go#newcode71
src/cmd/godoc/zip.go:71: 
How common is it that providers will want a nil Sys() method?

If (quite) common can't we have two types, one with and one without, so people
who wish to imply a nil here don't have to create these stub functions?

[bradfitz, 2012-02-02 19:50:17]

On Thu, Feb 2, 2012 at 11:40 AM, <cw@f00f.org> wrote:

>
>
http://codereview.appspot.com/**5448079/diff/23001/src/cmd/**godoc/zip.go<htt...
> File src/cmd/godoc/zip.go (right):
>
> http://codereview.appspot.com/**5448079/diff/23001/src/cmd/**
>
godoc/zip.go#newcode71<http://codereview.appspot.com/5448079/diff/23001/src/cmd/godoc/zip.go#newcode71>
> src/cmd/godoc/zip.go:71:
> How common is it that providers will want a nil Sys() method?
>
> If (quite) common can't we have two types, one with and one without, so
> people who wish to imply a nil here don't have to create these stub
> functions?
>

I want it clear that this isn't a required part of the otherwise
mostly-clean os.FileInfo interface.

[rsc, 2012-02-02 19:50:28]

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go
File src/pkg/os/types.go (right):

http://codereview.appspot.com/5448079/diff/18001/src/pkg/os/types.go#newcode22
src/pkg/os/types.go:22: Sys() interface{}   // underlying data source
// underlying data source (can return nil)

[cw, 2012-02-02 20:15:43]

> I want it clear that this isn't a required part of the otherwise
> mostly-clean os.FileInfo interface.

in which case the change to src/cmd/godoc/zip.g shouldn't ne needed

[rsc, 2012-02-02 20:38:12]

On Thu, Feb 2, 2012 at 15:15, Chris Wedgwood <cw@f00f.org> wrote:
> in which case the change to src/cmd/godoc/zip.g shouldn't ne needed

sorry, but it is required.  otherwise we have two types
and that is the kind of duplication we are actively trying
to avoid.  it can return nil.

russ

[niemeyer, 2012-02-02 20:53:47]

Hello cw@f00f.org, bradfitz@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-02-02 22:49:01]

LGTM

[niemeyer, 2012-02-03 02:16:28]

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

os: turn FileStat.Sys into a method on FileInfo

This reduces the overhead necessary to work with OS-specific
file details, hides the implementation of FileStat, and
preserves the implementation-specific nature of Sys.

Expressions such as:

  stat.(*os.FileInfo).Sys.(*syscall.Stat_t).Uid
  fi1.(*os.FileStat).SameFile(fi2.(*os.FileStat))

Are now spelled as::

  stat.Sys().(*syscall.Stat_t).Uid
  os.SameFile(fi1, fi2)

R=cw, bradfitz, rsc
CC=golang-dev
http://codereview.appspot.com/5448079