|
|
Created:
11 years, 2 months ago by adg Modified:
11 years, 1 month ago Reviewers:
CC:
rsc, golang-dev Visibility:
Public. |
Descriptionregexp: add (*Regexp).Longest
Fixes issue 3696.
Patch Set 1 #Patch Set 2 : diff -r a55bd9f21507 https://code.google.com/p/go #
Total comments: 4
Patch Set 3 : diff -r 36925180b9ef https://code.google.com/p/go #
MessagesTotal messages: 16
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
I named the method LeftmostLongest as per the issue, but perhaps the name Longest would be better, as all match types are leftmost. This might also be more natural and easily located as func CompileLongest(expr string) (*Regexp, error) Thoughts?
Sign in to reply to this message.
Longest sounds good (drop Leftmost). CompileLongest is not great because next you need MustCompileLongest.
Sign in to reply to this message.
Can it return the *Regexp to allow the builder pattern? That's pretty common for regular expressions.
Sign in to reply to this message.
On 17 January 2013 15:44, David Symonds <dsymonds@golang.org> wrote: > Can it return the *Regexp to allow the builder pattern? That's pretty > common for regular expressions. > Happy to do this too. Would be handy for cases like: var foo = MustCompile(`foo`).Longest()
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, rsc@golang.org, dsymonds@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 17 January 2013 15:52, Andrew Gerrand <adg@golang.org> wrote: > > On 17 January 2013 15:44, David Symonds <dsymonds@golang.org> wrote: > >> Can it return the *Regexp to allow the builder pattern? That's pretty >> common for regular expressions. >> > > Happy to do this too. Would be handy for cases like: > > var foo = MustCompile(`foo`).Longest() > The question about the builder pattern: does it mutate or copy the receiver?
Sign in to reply to this message.
On Thu, Jan 17, 2013 at 3:52 PM, Andrew Gerrand <adg@golang.org> wrote: > On 17 January 2013 15:44, David Symonds <dsymonds@golang.org> wrote: >> >> Can it return the *Regexp to allow the builder pattern? That's pretty >> common for regular expressions. > > > Happy to do this too. Would be handy for cases like: > > var foo = MustCompile(`foo`).Longest() Yes, that's the exact situation I'm thinking of.
Sign in to reply to this message.
On Thu, Jan 17, 2013 at 3:53 PM, Andrew Gerrand <adg@golang.org> wrote: > The question about the builder pattern: does it mutate or copy the receiver? It usually doesn't matter. It's most often used with immutable objects, so it would normally return a copy, but here it seems fine to mutate in-place.
Sign in to reply to this message.
https://codereview.appspot.com/7133051/diff/6001/src/pkg/regexp/exec_test.go File src/pkg/regexp/exec_test.go (right): https://codereview.appspot.com/7133051/diff/6001/src/pkg/regexp/exec_test.go#... src/pkg/regexp/exec_test.go:712: re, err := Compile(`a(|b)`) I found it curious that this test breaks if this were `ab?`, which is an equivalent expression. I wonder if that is a bug. The traditional definitions of leftmost longest don't restrict to alternations. https://codereview.appspot.com/7133051/diff/6001/src/pkg/regexp/regexp.go File src/pkg/regexp/regexp.go (right): https://codereview.appspot.com/7133051/diff/6001/src/pkg/regexp/regexp.go#new... src/pkg/regexp/regexp.go:133: // Longest sets the match semantics of the regexp to leftmost-longest. I think s/-//, since this is not being used as a direct adjective.
Sign in to reply to this message.
The builder pattern is not very common in the Go libraries, and I don't expect this to be a commonly used function. My original intent was to mutate the Regexp but it might be better to make it a compilation parameter, meaning CompileLongest and MustCompileLongest. Those would give us maximum flexibility in the future, making it possible for the compilation to behave differently if needed. All that said, let's put this on hold for now, since the functionality being exposed apparently doesn't even work. Russ
Sign in to reply to this message.
On 2013/01/17 14:08:22, rsc wrote: > The builder pattern is not very common in the Go libraries, and I > don't expect this to be a commonly used function. My original intent > was to mutate the Regexp but it might be better to make it a > compilation parameter, meaning CompileLongest and MustCompileLongest. > Those would give us maximum flexibility in the future, making it > possible for the compilation to behave differently if needed. > > All that said, let's put this on hold for now, since the functionality > being exposed apparently doesn't even work. > > Russ Out of curiosity, is there a reason this couldn't be a (?...)-param? That seems to be the usual way to affect the behavior of a regexp as opposed to things like re.CaseInsensitive() or re.Multiline().
Sign in to reply to this message.
> Out of curiosity, is there a reason this couldn't be a (?...)-param? > That seems to be the usual way to affect the behavior of a regexp as > opposed to things like re.CaseInsensitive() or re.Multiline(). I don't want to contribute to the complexity of the regexp syntax world by creating our own incompatible syntax. There are enough of those already. Russ
Sign in to reply to this message.
LGTM https://codereview.appspot.com/7133051/diff/6001/src/pkg/regexp/exec_test.go File src/pkg/regexp/exec_test.go (right): https://codereview.appspot.com/7133051/diff/6001/src/pkg/regexp/exec_test.go#... src/pkg/regexp/exec_test.go:712: re, err := Compile(`a(|b)`) On 2013/01/17 05:02:19, dsymonds wrote: > I found it curious that this test breaks if this were `ab?`, which is an > equivalent expression. I wonder if that is a bug. The traditional definitions of > leftmost longest don't restrict to alternations. It sounds like everything is working and I just misread this comment. ab? is a(b|) not a(|b). That is, ? prefers to match, not not to match. For an expression equivalent to a(|b) you need a??. https://codereview.appspot.com/7133051/diff/6001/src/pkg/regexp/regexp.go File src/pkg/regexp/regexp.go (right): https://codereview.appspot.com/7133051/diff/6001/src/pkg/regexp/regexp.go#new... src/pkg/regexp/regexp.go:133: // Longest sets the match semantics of the regexp to leftmost-longest. On 2013/01/17 05:02:19, dsymonds wrote: > I think s/-//, since this is not being used as a direct adjective. // Longest makes future searches prefer the leftmost-longest match.
Sign in to reply to this message.
ping (lgtm)
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=3529e745c5d5 *** regexp: add (*Regexp).Longest Fixes issue 3696. R=rsc CC=golang-dev https://codereview.appspot.com/7133051
Sign in to reply to this message.
|