Re: code review 5975052: encoding/base64: fix panic when input len is not a mult... (issue 5975052)

I can add some test cases for strings like QQ=\n, but if there are no objections, I'd like to handle it on a
separate CL.

Sent from my iPad

On 01/04/2012, at 8:56, David Symonds <dsymonds@...> wrote:

> On Sun, Apr 1, 2012 at 2:58 AM,  <sougou@...> wrote:
> 
>> It seems like the code will not handle the case where there are line
>> breaks within the paddings, like "=\n=". Is that considered invalid?
> 
> Yeah, the previous code also broke on that. It's a bit fiddly to fix
> that, and I'm not too sure whether it's worth trying, but that's
> orthogonal to this CL.

code review 5971058: regexp: implement backtracking with pcre fallback (issue 5971058)

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev <at> googlegroups.com (cc: golang-dev <at> googlegroups.com),

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



Description:
regexp: implement backtracking with pcre fallback

Go's current regular expression package doesn't support all
popular patterns which programmers from other languages have
come to expect.  Notably, the Go regexp package doesn’t
implement the useful backreference functionality.  Without
that, users can't match "catcat" or "dogdog" with (cat|dog)\1
and would need to otherwise fall back to writing multiple
lines of code, possibly involving an "if" statement.

To permit more expressiveness than Go's strictly "regular"
regular expressions, this change permits falling back to the
wildly popular PCRE library when Go would previously return a
compile error.

This permits programmers to copy-paste common regular
expressions from forum sites, stackoverflow, etc. and rest
assured that they'll work as they would have in PHP or Ruby.

Go shouldn't let ideological fundamentalism about algorithmic

Re: code review 5971058: regexp: implement backtracking with pcre fallback (issue 5971058)

Re: code review 5971058: regexp: implement backtracking with pcre fallback (issue 5971058)

http://codereview.appspot.com/5971058/diff/1002/src/pkg/regexp/fallback.go
File src/pkg/regexp/fallback.go (right):

http://codereview.appspot.com/5971058/diff/1002/src/pkg/regexp/fallback.go#newcode1
src/pkg/regexp/fallback.go:1: // +build !cmd_go_bootstrap
put the build line after the copyright

http://codereview.appspot.com/5971058/diff/3001/src/pkg/regexp/regexp.go
File src/pkg/regexp/regexp.go (right):

http://codereview.appspot.com/5971058/diff/3001/src/pkg/regexp/regexp.go#newcode68
src/pkg/regexp/regexp.go:68: var fallbackCompile func(expr string, err
error) (*Regexp, error)
why fallback and not just override? If we're linking in pcre we might as
well use it. it is the industry standard after all

http://codereview.appspot.com/5971058/

Re: code review 5971058: regexp: implement backtracking with pcre fallback (issue 5971058)

http://codereview.appspot.com/5971058/diff/1002/src/pkg/regexp/fallback.go
File src/pkg/regexp/fallback.go (right):

http://codereview.appspot.com/5971058/diff/1002/src/pkg/regexp/fallback.go#newcode1
src/pkg/regexp/fallback.go:1: // +build !cmd_go_bootstrap
On 2012/04/01 04:07:49, adg wrote:
> put the build line after the copyright

Actually I was bit by this recently, when the copyright was too long.
Russ admitted it was a bit of a bug and recommended +build lines go
before the copyright.

http://codereview.appspot.com/5971058/diff/3001/src/pkg/regexp/regexp.go
File src/pkg/regexp/regexp.go (right):

http://codereview.appspot.com/5971058/diff/3001/src/pkg/regexp/regexp.go#newcode68
src/pkg/regexp/regexp.go:68: var fallbackCompile func(expr string, err
error) (*Regexp, error)
On 2012/04/01 04:07:49, adg wrote:
> why fallback and not just override? If we're linking in pcre we might
as well
> use it. it is the industry standard after all

It helps during bootstrapping, when cgo isn't available.  That's about
the only time we need a pure-Go implementation.

http://codereview.appspot.com/5971058/

Re: code review 5937043: cmd/dist: don't fail when mercurial is hg.bat on windows (issue 5937043)

On 2012/03/30 06:04:54, snaury wrote:

> ... Exactly the same behavior happens with
> go get, ...

I am under impression that "go get ..." will fail, if it is calling
hg.exe and hg.exe fails.

Are you saying, that if user do not have hg.exe installed, but uses
hg.bat instead, then "go get ..." will succeed, while Mercurial command
inside hg.bat fails? I am prepared to believe that. If that is the case,
perhaps we should do something to prevent this inconsistency. Maybe show
some warning about us using "batch" file, maybe refuse to execute it
altogether.

Alex

http://codereview.appspot.com/5937043/

Re: code review 5937043: cmd/dist: don't fail when mercurial is hg.bat on windows (issue 5937043)

On 2012/03/31 14:47:05, snaury wrote:

> I'm thinking about reworking this patch by porting LookPath from
package os to C
> so that it's done uniformly by both dist and the rest of go, then I
hope this
> would finally stop being about cmd.exe and hg.bat bugs.

But it is a hg issue at this moment. cmd/dist is bit specific in what it
does and who it calls. I am not sure all that extra code is worth the
effort. But, as I said earlier, I am OK either way.

Alex

http://codereview.appspot.com/5937043/

Re: code review 5585043: exp/fsnotify: Cross-platform filesystem notifications (issue 5585043)

On 2012/03/30 21:06:52, howeyc wrote:

> I have done some further investigation and it appears to me that
windows
> does not support attributes as a separate notification.

http://msdn.microsoft.com/en-us/library/windows/desktop/aa364391%28v=vs.85%29.aspx

I am not even sure what do "attributes" mean in Windows. If you mean
"who can act on file" and "what can be done" to a file, then Windows
files do have ACLs. But Windows ACLs can't be inquired about as per
current Go standard libraries. So, I suppose, we should not worry about
them. Yet.

> My thoughts are to roll IsAttribute() into IsModify() (and remove
> IsAttribute()) so the same functions are exported across all
> platforms.

It is hard for me to advise, because I do not use this facility myself.
But I don't think, if you leave IsAttribute() (which will return false
on windows) in, it is a big deal.

> Comments, concerns, etc?

I see no windows implementation in your proposal. What is your plan
about that? I thought, given that we have exp/winfsnotify package
already, you should be able to provide something that works. This way
your design will be closer to reality (I think "cross-platform" is the
key word in your CL).

Alex

http://codereview.appspot.com/5585043/

Re: code review 5971058: regexp: implement backtracking with pcre fallback (issue 5971058)

I like the approach, but what about running both native and PCRE in
parallel? There are regular expressions where each is faster, and you
could just select between whichever one finishes first. Go is all about
parallel programming, right?

http://codereview.appspot.com/5971058/

Re: code review 5971058: regexp: implement backtracking with pcre fallback (issue 5971058)