code review 4635082: exp/regexp/syntax: finish Regexp manipulation

exp/regexp/syntax: finish Regexp manipulation

Except for the inevitable bug fixes, the Regexp code is done.

[rsc, 2011-06-30 02:21:07]

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

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

[r, 2011-06-30 03:06:02]

i think simplify deserves a file of its own, so parsing and simplification are
easily seen as separate processes.

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

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/par...
src/pkg/exp/regexp/syntax/parse.go:305: // get flattened into a single node.
odd line break

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/par...
src/pkg/exp/regexp/syntax/parse.go:383: // Nothing to do - first iteration.
first iteration has i == 0. you mean something else.

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/par...
src/pkg/exp/regexp/syntax/parse.go:403: // Prepare for next iteration (if there
is one).
delete parenthetical remark

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/par...
src/pkg/exp/regexp/syntax/parse.go:436: // Nothing to do - first iteration.
first iteration has i == 0. you mean something else.

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/par...
src/pkg/exp/regexp/syntax/parse.go:455: // Prepare for next iteration (if there
is one).
ditto

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/par...
src/pkg/exp/regexp/syntax/parse.go:522: // leadingString returns the leading
literal string that re starts with.
s/re starts with/begins re/

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/par...
src/pkg/exp/regexp/syntax/parse.go:1041: // The caller must ensure that re1.Op >
re2.Op.
i'd expect re2.Op > re1.Op

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
File src/pkg/exp/regexp/syntax/regexp.go (right):

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp.go:63: func (x *Regexp) Equal(y *Regexp) bool {
doc comment. obvious though it is, it's still worthwhile.

under the covers, are we guaranteed that the regexps are simplified when we get
here?

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp.go:286: // and with various other
simplifications, like rewriting /(?:a+)+/ as /a+/.
s/ as / to /
s/like/such as/

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp.go:290: // for (x){1,2} is (x)(x)? but both
parentheses capture as $1.
is the returned regexp guaranteed a distinct copy?

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp.go:396: func simplify1(op Op, flags Flags, sub,
re *Regexp) *Regexp {
comment please, to make Simplify easier to follow

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
File src/pkg/exp/regexp/syntax/regexp_test.go (right):

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp_test.go:62: // captured subexpressions.  The
comments show a version fewer parens.
s/fewer/with fewer/

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp_test.go:75: // (Simplify used to not simplify
arguments to a {} repeat.)
when? this is new code. weird.

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp_test.go:119: // Empty string must be preserved
inside parens in order
s/string/&s/ ?

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp_test.go:120: // to make submatches work right,
so these are less
what does 'these' refer to?

http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
src/pkg/exp/regexp/syntax/regexp_test.go:121: // interesting than they used to
be.  ToString inserts
when was that?

[rsc, 2011-06-30 03:29:55]

PTAL.

Patch set 2 corresponds to what you commented on
but with the code shuffled into different files as requested.
So the diff from 2 to 3 is useful.

>
http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/par...
> src/pkg/exp/regexp/syntax/parse.go:1041: // The caller must ensure that
> re1.Op > re2.Op.
> i'd expect re2.Op > re1.Op

Renamed to dst, src.  The idea is that if src is simpler,
less copying will happen.

>
http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
> src/pkg/exp/regexp/syntax/regexp.go:63: func (x *Regexp) Equal(y
> *Regexp) bool {
> doc comment. obvious though it is, it's still worthwhile.
>
> under the covers, are we guaranteed that the regexps are simplified when
> we get here?

It's up to the caller.  When used during parsing, no.

>
http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
> src/pkg/exp/regexp/syntax/regexp.go:290: // for (x){1,2} is (x)(x)? but
> both parentheses capture as $1.
> is the returned regexp guaranteed a distinct copy?

No.  (Added comment.)

>
http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
> src/pkg/exp/regexp/syntax/regexp_test.go:75: // (Simplify used to not
> simplify arguments to a {} repeat.)
> when? this is new code. weird.
>
>
http://codereview.appspot.com/4635082/diff/2001/src/pkg/exp/regexp/syntax/reg...
> src/pkg/exp/regexp/syntax/regexp_test.go:121: // interesting than they
> used to be.  ToString inserts
> when was that?

Rewrote comments to remove references to this code's sordid C++ past.

Russ

[r, 2011-06-30 03:47:27]

LGTM

[rsc, 2011-06-30 14:26:24]

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

exp/regexp/syntax: finish Regexp manipulation

Except for the inevitable bug fixes, the Regexp code is done.

R=sam.thorogood, r
CC=golang-dev
http://codereview.appspot.com/4635082