code review 9293043: misc/vim: Allow multiple GOOS/GOARCH.

misc/vim: Allow multiple GOOS/GOARCH.

[mattn, 2013-05-08 01:59:39]

`go env GOROOT` returns multiple lines if there are some GOOS/GOARCH in pkg
directory.

[mattn, 2013-05-08 02:01:04]

`go env GOROOT` returns multiple lines if there are some GOOS/GOARCH in pkg
directory.

[mattn, 2013-05-08 02:02:33]

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

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

[mattn, 2013-05-08 02:02:53]

Oops sorry.

[dsymonds, 2013-05-08 02:50:00]

This is too late for go1.1, but can you please remind me of this after
go1.1 is released?

[Dominik Honnef, 2013-05-08 09:15:57]

Hm, I can't reproduce the 

> returns multiple lines if there are some GOOS/GOARCH in pkg directory.

thing here. What exactly is up with that?

[mattn, 2013-05-08 09:28:35]

Try to create directory in pkg.

$ ls
linux_amd64

$ mkdir linux_amd64_tmp

This make break.

On Wednesday, May 8, 2013 6:15:57 PM UTC+9, Dominik Honnef wrote:
>
> Hm, I can't reproduce the 
>
> > returns multiple lines if there are some GOOS/GOARCH in pkg directory. 
>
> thing here. What exactly is up with that? 
>
> https://codereview.appspot.com/9293043/ 
>

[Dominik Honnef, 2013-05-08 09:35:54]

dominikh@dominikh-pc $ go env GOROOT
/home/dominikh/go
dominikh@dominikh-pc $ mkdir $GOROOT/pkg/linux_amd64_tmp
dominikh@dominikh-pc $ go env GOROOT
/home/dominikh/go
dominikh@dominikh-pc $ go version
go version devel +aa1ff597baa5 Wed Apr 03 20:32:50 2013 +1100 linux/amd64

For what it's worth, I already had directories for all GOOS/GOARCH combinations
in /pkg/, since I have cross compilation for all of those set up. So I can't
still reproduce it.

[mattn, 2013-05-08 10:10:20]

Look here

https://codereview.appspot.com/9293043/diff/6001/misc/vim/autoload/go/complet...

Probably, your environment doesn't set "*" to s:goos or s:goarch.
On my environment, it is set as '*'. Then,

  expand(dir . '/pkg/' . s:goos . '_' . s:goarch)

return multiple-line.

On Wednesday, May 8, 2013 6:35:54 PM UTC+9, Dominik Honnef wrote:
>
> dominikh@dominikh-pc $ go env GOROOT 
> /home/dominikh/go 
> dominikh@dominikh-pc $ mkdir $GOROOT/pkg/linux_amd64_tmp 
> dominikh@dominikh-pc $ go env GOROOT 
> /home/dominikh/go 
> dominikh@dominikh-pc $ go version 
> go version devel +aa1ff597baa5 Wed Apr 03 20:32:50 2013 +1100 
> linux/amd64 
>
> For what it's worth, I already had directories for all GOOS/GOARCH 
> combinations in /pkg/, since I have cross compilation for all of those 
> set up. So I can't still reproduce it. 
>
> https://codereview.appspot.com/9293043/ 
>

[mattn, 2013-07-02 02:18:43]

ping

[dsymonds, 2013-07-02 03:02:15]

https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
File misc/vim/autoload/go/complete.vim (right):

https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
misc/vim/autoload/go/complete.vim:47: let pathsep = (has('win32')||has('win64'))
? ';' : ':'
may as well use existing vars for this. please frame it to make windows the
exceptional case too.

let sep = ':'
if s:goos == 'windows'
  let sep = ';'
endif

https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
misc/vim/autoload/go/complete.vim:60: let root = split(expand(dir . '/pkg/' .
s:goos . '_' . s:goarch), "\n")
what is this splitting? shouldn't this expand to a single thing?

if this is to permit this not matching, make that explicit instead.

let root = expand(...)
if root == ''
  continue
endif

[mattn, 2013-07-02 03:20:24]

Hello golang-dev@googlegroups.com, dsymonds@golang.org, dominik.honnef@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[mattn, 2013-07-02 03:20:49]

https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
File misc/vim/autoload/go/complete.vim (right):

https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
misc/vim/autoload/go/complete.vim:47: let pathsep = (has('win32')||has('win64'))
? ';' : ':'
On 2013/07/02 03:02:16, dsymonds wrote:
> may as well use existing vars for this. please frame it to make windows the
> exceptional case too.
> 
> let sep = ':'
> if s:goos == 'windows'
>   let sep = ';'
> endif

Done.

https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
misc/vim/autoload/go/complete.vim:60: let root = split(expand(dir . '/pkg/' .
s:goos . '_' . s:goarch), "\n")
Yes, this return multiple-lines if several directries are exists in /pkg/. The
s:goarch is '*' on my environment.

On 2013/07/02 03:02:16, dsymonds wrote:
> what is this splitting? shouldn't this expand to a single thing?
> 
> if this is to permit this not matching, make that explicit instead.
> 
> let root = expand(...)
> if root == ''
>   continue
> endif

[dsymonds, 2013-07-02 04:08:57]

https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
File misc/vim/autoload/go/complete.vim (right):

https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
misc/vim/autoload/go/complete.vim:60: let root = split(expand(dir . '/pkg/' .
s:goos . '_' . s:goarch), "\n")
On 2013/07/02 03:20:50, mattn wrote:
> Yes, this return multiple-lines if several directries are exists in /pkg/. The
> s:goarch is '*' on my environment.
> 
> On 2013/07/02 03:02:16, dsymonds wrote:
> > what is this splitting? shouldn't this expand to a single thing?
> > 
> > if this is to permit this not matching, make that explicit instead.
> > 
> > let root = expand(...)
> > if root == ''
> >   continue
> > endif
> 

okay, that wasn't obvious. Add a comment.

" this may expand to multiple lines

[mattn, 2013-07-02 04:53:20]

On 2013/07/02 04:08:57, dsymonds wrote:
>
https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
> File misc/vim/autoload/go/complete.vim (right):
> 
>
https://codereview.appspot.com/9293043/diff/16001/misc/vim/autoload/go/comple...
> misc/vim/autoload/go/complete.vim:60: let root = split(expand(dir . '/pkg/' .
> s:goos . '_' . s:goarch), "\n")
> On 2013/07/02 03:20:50, mattn wrote:
> > Yes, this return multiple-lines if several directries are exists in /pkg/.
The
> > s:goarch is '*' on my environment.
> > 
> > On 2013/07/02 03:02:16, dsymonds wrote:
> > > what is this splitting? shouldn't this expand to a single thing?
> > > 
> > > if this is to permit this not matching, make that explicit instead.
> > > 
> > > let root = expand(...)
> > > if root == ''
> > >   continue
> > > endif
> > 
> 
> okay, that wasn't obvious. Add a comment.
> 
> " this may expand to multiple lines

Done

[dsymonds, 2013-07-02 05:23:02]

LGTM

[dsymonds, 2013-07-02 05:24:15]

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

misc/vim: Allow multiple GOOS/GOARCH.

R=golang-dev, dsymonds, dominik.honnef
CC=golang-dev
https://codereview.appspot.com/9293043

Committer: David Symonds <dsymonds@golang.org>