code review 4612041: exp/regexp/syntax: more escapes, character classes

exp/regexp/syntax: more escapes, character classes

Still TODO: parsing optimizations

make_perl_groups.pl is copied with minimal modifications
(just to generate Go syntax instead of C++) from RE2.
Google Inc is "The RE2 Author" of that file and is one of
the Go Authors, so copyright changed to the Go Authors instead.

[rsc, 2011-06-13 15:12:11]

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

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

[rsc, 2011-06-13 15:53:07]

Added Unicode; it was just a few lines.
Now the only TODO is the parsing optimizations.
That will be a separate CL.

[fvb, 2011-06-13 19:09:47]

http://codereview.appspot.com/4612041/diff/1003/src/pkg/exp/regexp/syntax/par...
File src/pkg/exp/regexp/syntax/parse.go (right):

http://codereview.appspot.com/4612041/diff/1003/src/pkg/exp/regexp/syntax/par...
src/pkg/exp/regexp/syntax/parse.go:756: // character or use \008.
Don't you mean either \x08 or \010 here?

[rsc, 2011-06-13 19:30:47]

> Don't you mean either \x08 or \010 here?

Fixed, thanks.  RE2 has the same buggy comment.
Will fix there too.

[r, 2011-06-14 03:56:42]

LGTM

what a load of nonsense.

i do think this could be significantly cleaner with some helper methods like
accept() and peek(), but we can discuss that in another round.

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
File src/pkg/exp/regexp/syntax/parse.go (right):

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
src/pkg/exp/regexp/syntax/parse.go:1: // Copyright 2011 The Go Authors.  All
rights reserved.
not in this cl but if the parser kept a running input string and maybe location,
the "rest" return value could go away everywhere. you've got wholeRegexp; all
you need is a position.

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
src/pkg/exp/regexp/syntax/parse.go:232: for t != "" {
optional:
this is a monster function and nothing in the loop matters outside. p goes in, p
comes out. how about making the loop a method? then break BigSwitch becomes
return and the code is a little more concise. all the returns in here are
errors, so that helps too

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
src/pkg/exp/regexp/syntax/parse.go:348: // \Q ... \E: the ... is always literals
wow

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
src/pkg/exp/regexp/syntax/parse.go:560: // is a valid capture name. 
[A-Za-z0-9_]+.
s/\. /:/

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
src/pkg/exp/regexp/syntax/parse.go:690: case '0':
you're already a little incompatible.  can you be more incompatible by making
these escapes go-like?  probably not, but i thought i'd ask. it bothers me that
\xff means different things in the different worlds.

failing that, i'd consider dropping them altogether.

i don't expect to win this debate.

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
src/pkg/exp/regexp/syntax/parse.go:856: if p.flags&UnicodeGroups == 0 || len(s)
< 2 || s[0] != '\\' || s[1] != 'p' && s[1] != 'P' {
might was well use strings.HasPrefix for some of these.

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
src/pkg/exp/regexp/syntax/parse.go:860: // Commited to parse or return error.
s/t/tt/

[rh, 2011-06-14 14:00:51]

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/ma...
File src/pkg/exp/regexp/syntax/make_perl_groups.pl (right):

http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/ma...
src/pkg/exp/regexp/syntax/make_perl_groups.pl:2: # Copyright 2008 The Go
Authors.  All rights reserved.
2011?

[rsc, 2011-06-14 14:31:36]

>
http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/ma...
> src/pkg/exp/regexp/syntax/make_perl_groups.pl:2: # Copyright 2008 The Go
> Authors.  All rights reserved.
> 2011?

no, this one is from
http://code.google.com/p/re2/source/browse/re2/make_perl_groups.pl

[rsc, 2011-06-14 18:29:51]

> i do think this could be significantly cleaner with some helper methods
> like accept() and peek(), but we can discuss that in another round.

sounds good.

> this is a monster function and nothing in the loop matters outside. p
> goes in, p comes out. how about making the loop a method? then break
> BigSwitch becomes return and the code is a little more concise. all the
> returns in here are errors, so that helps too

tried this.  even in a separate function i still had break BigSwitch
because i want to have just one successful return in the function,
at the end of the switch.


>
http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
> src/pkg/exp/regexp/syntax/parse.go:560: // is a valid capture name.
> [A-Za-z0-9_]+.
> s/\. /:/

done

>
http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
> src/pkg/exp/regexp/syntax/parse.go:856: if p.flags&UnicodeGroups == 0 ||
> len(s) < 2 || s[0] != '\\' || s[1] != 'p' && s[1] != 'P' {
> might was well use strings.HasPrefix for some of these.

maybe, but for example here i'd have to call it twice.
they all seem short enough not to bother.

>
http://codereview.appspot.com/4612041/diff/16001/src/pkg/exp/regexp/syntax/pa...
> src/pkg/exp/regexp/syntax/parse.go:860: // Commited to parse or return
> error.
> s/t/tt/

done

[rsc, 2011-06-14 18:30:12]

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

exp/regexp/syntax: more escapes, character classes

Still TODO: parsing optimizations

make_perl_groups.pl is copied with minimal modifications
(just to generate Go syntax instead of C++) from RE2.
Google Inc is "The RE2 Author" of that file and is one of
the Go Authors, so copyright changed to the Go Authors instead.

R=sam.thorogood, r, fvbommel, robert.hencke
CC=golang-dev
http://codereview.appspot.com/4612041