code review 3399042: token/position.go: provide FileSet.File(), minor optimi...

token/position.go: provide FileSet.File(), minor optimizations

[gri, 2010-12-08 00:05:05]

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

I'd like you to review this change.

[rsc1, 2010-12-08 01:19:43]

LGTM

http://codereview.appspot.com/3399042/diff/1003/src/pkg/go/token/position.go
File src/pkg/go/token/position.go (right):

http://codereview.appspot.com/3399042/diff/1003/src/pkg/go/token/position.go#...
src/pkg/go/token/position.go:173: // Methods of files are not synchronized; only
one goroutine
I don't think this needs to be said; it's the default
behavior of any mutable data, and I don't think we
should start adding this to every non-thread-safe
data structure (bytes.Buffer, vector.Vector, etc).

But I'm confused: I don't see any methods that aren't synchronized.

[gri, 2010-12-08 05:49:48]

On Tue, Dec 7, 2010 at 5:19 PM, <rsc@google.com> wrote:

> LGTM
>
>
>
>
> http://codereview.appspot.com/3399042/diff/1003/src/pkg/go/token/position.go
> File src/pkg/go/token/position.go (right):
>
>
>
http://codereview.appspot.com/3399042/diff/1003/src/pkg/go/token/position.go#...
> src/pkg/go/token/position.go:173: // Methods of files are not
> synchronized; only one goroutine
> I don't think this needs to be said; it's the default
> behavior of any mutable data, and I don't think we
> should start adding this to every non-thread-safe
> data structure (bytes.Buffer, vector.Vector, etc).
>

I only mentioned it here to emphasize the difference to FileSet - but sure,
it's not needed. Removed.

>
> But I'm confused: I don't see any methods that aren't synchronized.


Methods on FileSet should be synchronized; other methods on Files should
not. However, File methods accessing the line information are synchronized
against the corresponding file set because the file's line information may
be accessed via the file set (through FileSet.Position); other File methods
are not synchronized.

That said, I am wondering now if that is the correct approach. It may be
better to never synchronize and let the app do the right thing. The obvious
case is godoc where multiple requests may access the file set's position
information.

Since this CL doesn't change that I will go ahead and submit it, but revisit
the approach tomorrow.

- gri


>
> http://codereview.appspot.com/3399042/
>

[gri, 2010-12-08 05:50:12]

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

token/position.go: provide FileSet.File(), minor optimizations

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

Committer: Robert Griesemer <gri@golang.org>