code review 4249064: path: work for windows.

path: work for windows.

[mattn, 2011-03-09 05:26:10]

Hello golang-dev@googlegroups.com,

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

[brainman, 2011-03-09 06:12:27]

Please, check your changes with godoc to see if it resolves our windows path
problems. Thank you.

Alex

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match.go
File src/pkg/path/filepath/match.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:45: return strings.IndexFunc(name,
isSeparatorFunc) < 0, nil
Please, leave original code. If Separator == '\' on Windows and Clean function
'normalize' file path first, then it should work.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
File src/pkg/path/filepath/path_unix.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
src/pkg/path/filepath/path_unix.go:15: func IsSeparator(c uint8) bool {
Remember Russ's suggested function names:

Base
Ext
Glob
IsAbs
Join
Match
Split
Walk
Visitor

There is no IsSeparator there. Let's stick with what Russ suggested for now.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
File src/pkg/path/filepath/path_windows.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:17: return strings.Index("\\/", string(c))
!= -1
Let's use '\' only.

Just change Clean function to convert all '/' into '\' as suggested by Russ.

[rsc, 2011-03-09 06:16:18]

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match.go
File src/pkg/path/filepath/match.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:45: return strings.IndexFunc(name,
isSeparatorFunc) < 0, nil
Please revert the changes in this file.  There's much more broken here
than the handling of separators.  Notice that the grammar for Match
says that \ is used for escaping special characters.

Let's worry about Match in a different CL.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match_t...
File src/pkg/path/filepath/match_test.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match_t...
src/pkg/path/filepath/match_test.go:82: s = filepath.FromSlash(s)
Revert the changes in this file too.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:143: i := strings.LastIndexFunc(path,
isSeparatorFunc)
might as well just do

i := len(path)-1
for i >= 0 && !isSeparator(path[i]) {
    i--
}

and avoid needing isSeparatorFunc.  It will be faster too.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:260: if i := strings.LastIndexFunc(path,
isSeparatorFunc); i >= 0 {
Same rewrite.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:270: // isSeparatorFunc is called for checking
whether it is separator.
delete

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
File src/pkg/path/filepath/path_unix.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
src/pkg/path/filepath/path_unix.go:14: // IsSeparator returns true if c is a
directory separator character.
rename to isSeparator.

I do not want to expand the public API.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
src/pkg/path/filepath/path_unix.go:24: // pathPrefix return leading volume name.
Call this volumeName.

// volumeName returns the leading volume name on Windows.
// It returns "" on Unix.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
File src/pkg/path/filepath/path_windows.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:14: // IsSeparator returns true if c is a
directory separator character.
isSeparator

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:17: return strings.Index("\\/", string(c))
!= -1
This implementation contains a memory allocation (string(c))
as well as a function call for a string search, just to test two choices.

return c == '\\' || c == '/'

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:17: return strings.Index("\\/", string(c))
!= -1
On 2011/03/09 06:12:27, brainman wrote:
> Let's use '\' only.
> 
> Just change Clean function to convert all '/' into '\' as suggested by Russ.

I think we will need functions to know about both.
It's not practical to call Clean in, say, Ext.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:22: return strings.Index(":", string(c))
!= -1
return c == ':'

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:27: if path == "" {
return path != "" && (volumePrefix(path) != "" || isSeparator(path[0]))

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:51: c := int(path[0])
not clear why the int conversion is here.

c := path[0]

would work just as well

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:53: (('0' <= c && c <= '9') || ('a' <= c
&& c <= 'z') ||
over parenthesized.  drop parens around && expressions

[mattn, 2011-03-09 06:43:24]

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

Please take another look.

[mattn, 2011-03-09 06:43:35]

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match.go
File src/pkg/path/filepath/match.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:45: return strings.IndexFunc(name,
isSeparatorFunc) < 0, nil
On 2011/03/09 06:12:27, brainman wrote:
> Please, leave original code. If Separator == '\' on Windows and Clean function
> 'normalize' file path first, then it should work.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:45: return strings.IndexFunc(name,
isSeparatorFunc) < 0, nil
For passing this test without conversion on windows.

We need to return slashed paths. really?

On 2011/03/09 06:16:18, rsc wrote:
> Please revert the changes in this file.  There's much more broken here
> than the handling of separators.  Notice that the grammar for Match
> says that \ is used for escaping special characters.
> 
> Let's worry about Match in a different CL.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match_t...
File src/pkg/path/filepath/match_test.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/match_t...
src/pkg/path/filepath/match_test.go:82: s = filepath.FromSlash(s)
On 2011/03/09 06:16:18, rsc wrote:
> Revert the changes in this file too.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:143: i := strings.LastIndexFunc(path,
isSeparatorFunc)
On 2011/03/09 06:16:18, rsc wrote:
> might as well just do
> 
> i := len(path)-1
> for i >= 0 && !isSeparator(path[i]) {
>     i--
> }
> 
> and avoid needing isSeparatorFunc.  It will be faster too.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:260: if i := strings.LastIndexFunc(path,
isSeparatorFunc); i >= 0 {
On 2011/03/09 06:16:18, rsc wrote:
> Same rewrite.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:270: // isSeparatorFunc is called for checking
whether it is separator.
On 2011/03/09 06:16:18, rsc wrote:
> delete

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
File src/pkg/path/filepath/path_unix.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
src/pkg/path/filepath/path_unix.go:14: // IsSeparator returns true if c is a
directory separator character.
On 2011/03/09 06:16:18, rsc wrote:
> rename to isSeparator.
> 
> I do not want to expand the public API.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
src/pkg/path/filepath/path_unix.go:15: func IsSeparator(c uint8) bool {
On 2011/03/09 06:12:27, brainman wrote:
> Remember Russ's suggested function names:
> 
> Base
> Ext
> Glob
> IsAbs
> Join
> Match
> Split
> Walk
> Visitor
> 
> There is no IsSeparator there. Let's stick with what Russ suggested for now.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_un...
src/pkg/path/filepath/path_unix.go:24: // pathPrefix return leading volume name.
On 2011/03/09 06:16:18, rsc wrote:
> Call this volumeName.
> 
> // volumeName returns the leading volume name on Windows.
> // It returns "" on Unix.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
File src/pkg/path/filepath/path_windows.go (right):

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:14: // IsSeparator returns true if c is a
directory separator character.
On 2011/03/09 06:16:18, rsc wrote:
> isSeparator

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:17: return strings.Index("\\/", string(c))
!= -1
On 2011/03/09 06:12:27, brainman wrote:
> Let's use '\' only.
> 
> Just change Clean function to convert all '/' into '\' as suggested by Russ.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:17: return strings.Index("\\/", string(c))
!= -1
On 2011/03/09 06:16:18, rsc wrote:
> This implementation contains a memory allocation (string(c))
> as well as a function call for a string search, just to test two choices.
> 
> return c == '\\' || c == '/'
> 

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:17: return strings.Index("\\/", string(c))
!= -1
On 2011/03/09 06:16:18, rsc wrote:
> This implementation contains a memory allocation (string(c))
> as well as a function call for a string search, just to test two choices.
> 
> return c == '\\' || c == '/'
> 

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:17: return strings.Index("\\/", string(c))
!= -1
On 2011/03/09 06:12:27, brainman wrote:
> Let's use '\' only.
> 
> Just change Clean function to convert all '/' into '\' as suggested by Russ.

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:27: if path == "" {
On 2011/03/09 06:16:18, rsc wrote:
> return path != "" && (volumePrefix(path) != "" || isSeparator(path[0]))

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:51: c := int(path[0])
On 2011/03/09 06:16:18, rsc wrote:
> not clear why the int conversion is here.
> 
> c := path[0]
> 
> would work just as well

Done.

http://codereview.appspot.com/4249064/diff/6001/src/pkg/path/filepath/path_wi...
src/pkg/path/filepath/path_windows.go:53: (('0' <= c && c <= '9') || ('a' <= c
&& c <= 'z') ||
On 2011/03/09 06:16:18, rsc wrote:
> over parenthesized.  drop parens around && expressions

Done.

[rsc, 2011-03-09 06:46:06]

http://codereview.appspot.com/4249064/diff/4008/src/pkg/path/filepath/match.go
File src/pkg/path/filepath/match.go (right):

http://codereview.appspot.com/4249064/diff/4008/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:62: for i := 0; i < len(name) &&
!isSeparator(name[i]); i++ {
You are still calling isSeparator.
Please revert *all* changes to this file.
If you need to disable the test on Windows, that's fine.

[mattn, 2011-03-09 07:05:01]

http://codereview.appspot.com/4249064/diff/4008/src/pkg/path/filepath/match.go
File src/pkg/path/filepath/match.go (right):

http://codereview.appspot.com/4249064/diff/4008/src/pkg/path/filepath/match.g...
src/pkg/path/filepath/match.go:62: for i := 0; i < len(name) &&
!isSeparator(name[i]); i++ {
On 2011/03/09 06:46:06, rsc wrote:
> You are still calling isSeparator.
> Please revert *all* changes to this file.
> If you need to disable the test on Windows, that's fine.

Done.

[mattn, 2011-03-10 01:30:23]

*** Abandoned ***

[mattn, 2011-03-10 01:54:19]

On 2011/03/10 01:30:23, mattn wrote:
> *** Abandoned ***

I had mistake to update issue.
I just re-oepned. sorry.

[brainman, 2011-03-10 05:24:25]

LGTM. But I won't know until it is used somewhere in real life app, like godoc.

http://codereview.appspot.com/4249064/diff/1008/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/4249064/diff/1008/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:34: func Clean(path string) string {
Russ, you said: "Clean should convert / to \ on Windows.". Are we going to do it
here?

http://codereview.appspot.com/4249064/diff/1008/src/pkg/path/filepath/path_te...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4249064/diff/1008/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:71: if s :=
filepath.ToSlash(filepath.Clean(test.path)); s != test.result {
Should we be using ToSlash/FromSlash here?

[rsc, 2011-03-11 16:26:33]

LGTM

I am happy with this.
We can worry about Clean in a separate CL.
Leaving for Alex.

Russ

[brainman, 2011-03-12 08:32:07]

On 2011/03/11 16:26:33, rsc wrote:
> We can worry about Clean in a separate CL.

Sorry, but the build fails.

cd $GOROOT/src/pkg/go/scanner
make test

fails with:

gotest
make[1]: Entering directory `/C/MinGW/go/src/pkg/go/scanner'
rm -f _test/go/scanner.a
make[1]: Leaving directory `/C/MinGW/go/src/pkg/go/scanner'
make[1]: Entering directory `/C/MinGW/go/src/pkg/go/scanner'
rm -f _test/go/scanner.a
gopack grc _test/go/scanner.a _gotest_.8 
make[1]: Leaving directory `/C/MinGW/go/src/pkg/go/scanner'
--- FAIL: scanner.TestLineComments (0.0 seconds)
	bad filename for "line100": got dir\File1.go, expected dir/File1.go
	bad filename for "line200": got dir\File2.go, expected dir/File2.go
	bad filename for "line42": got dir\foo, expected dir/foo
	bad filename for "line44": got dir\foo, expected dir/foo
	bad filename for "line46": got dir\foo, expected dir/foo
	bad filename for "line48": got dir\foo, expected dir/foo
	bad filename for "line42": got dir\bar, expected /bar
	bad filename for "line42": got dir\foo, expected dir/foo
	bad filename for "line100": got dir\a\b\c\File1.go, expected dir/a/b/c/File1.go
FAIL
make: *** [test] Error 1

[brainman, 2011-03-15 00:56:14]

On 2011/03/12 08:32:07, brainman wrote:
> On 2011/03/11 16:26:33, rsc wrote:
> > We can worry about Clean in a separate CL.
> 
> Sorry, but the build fails.
> 

Russ, I'm OK to submit this CL. But I would like to know how to fix the build
after.

This:

diff -r 34118d2bd7d3 src/pkg/go/scanner/scanner.go
--- a/src/pkg/go/scanner/scanner.go	Mon Mar 14 14:12:37 2011 -0700
+++ b/src/pkg/go/scanner/scanner.go	Tue Mar 15 11:52:14 2011 +1100
@@ -181,7 +181,7 @@
 			if line, err := strconv.Atoi(string(text[i+1:])); err == nil && line > 0 {
 				// valid //line filename:line comment;
 				filename := filepath.Clean(string(text[len(prefix):i]))
-				if filename[0] != '/' {
+				if !filepath.IsAbs(filename) {
 					// make filename relative to current directory
 					filename = filepath.Join(S.dir, filename)
 				}

will fix "absolute or relative" path detection. But the rest are due to test
code expects file paths to have / as a delimiter. How do you propose to change
the test code to make it pass.

Thank you.

Alex

[rsc1, 2011-03-15 00:59:06]

I think just change the segments table in go/scanner/scanner_test.go
to use calls to filepath.Join("dir", "TestLineComments") etc.

[rsc, 2011-03-15 00:59:34]

Also you should be able to send the CL fixing the
go/scanner test *before* submitting this one, so that
the build never breaks.

Russ

[brainman, 2011-03-16 23:41:29]

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

path: work for windows.

R=brainman, rsc, rsc1
CC=golang-dev
http://codereview.appspot.com/4249064

Committer: Alex Brainman <alex.brainman@gmail.com>