Add regex match iterator #2109

autodidaddict · 2017-07-30T00:07:04Z

Add regex match iterator Issue 2093, RFC 044

SeanTAllen · 2017-07-30T02:02:24Z

packages/regex/match_iter.pony

@@ -0,0 +1,38 @@
+class MatchIterator is Iterator[Match]


per the pony file naming conventions this file shoudl be called match_interator.pony

SeanTAllen · 2017-07-30T02:03:24Z

packages/regex/_test.pony

@@ -61,6 +63,43 @@ class iso _TestEq is UnitTest
    h.assert_true(r == "1234", """ \d+ =~ "1234" """)
    h.assert_true(r != "asdf", """ \d+ !~ "asdf" """)

+class iso _TestMatchIter is UnitTest


I think this should be _TestMatchIterator or _TestMatches

SeanTAllen · 2017-07-30T02:03:52Z

packages/regex/match_iter.pony

+    """
+    let m = _regex(_subject, _offset)?
+    _offset = m.end_pos() + 1 
+    m


can you add a newline at the end of the file?

SeanTAllen · 2017-07-30T02:04:20Z

This looks pretty good, couple small changes.

jemc · 2017-07-31T01:14:33Z

packages/regex/match_iterator.pony

+      false
+    end 
+
+  fun ref next() : Match? =>


Style-wise, the : here should not have a preceding space, and the ? should.

When in doubt on style, check the style guide as well as surrounding code.

jemc · 2017-07-31T01:15:48Z

packages/regex/match_iterator.pony

+    let m = _regex(_subject, _offset)?
+    _offset = m.end_pos() + 1 
+    m
+


This little symbol on GitHub is indicating that this file is missing an empty line at the end of it.

I don't see the little symbol, and there is a line at the end of the file.

That's odd - it may be indicating that there is extra whitespace on the line at the end of the file?

It means "there's no newline at the end of the file".

jemc · 2017-07-31T01:16:41Z

packages/regex/regex.pony

+  fun matches(subject: String): MatchIterator =>
+    """ 
+    Creates a match iterator from the regular expression that will iterate
+    over the supplied subject returning matches


Could you add a period at the end of this sentence?

jemc · 2017-07-31T01:18:14Z

I'm a little bit concerned that the implementation will execute every match twice in a for loop, where both has_next and next are run.

However, that could be optimized by somebody who cared enough about the performance boost to do it, so I'm not necessarily averse to leaving it as-is for now.

plietar · 2017-07-31T18:27:19Z

How about removing the has_next method from Iterator, and changing for loop desugaring to rely only on the partialness of the next method to quit the loop ?

It would avoid this sort of problem, where checking if there is a next element can be expensive. There are even cases where it is impossible to check if there's a next value without consuming it.

The workaround is kind of awkward and requires caching the value, such as in the FileLines iterator. I also ran into this a while ago when writing a pony wrapper around a Rust iterator.

I've just had a go at implementing it in the compiler and stdlib. All uses of has_next can be replaced quite easily. In many cases it even simplifies both the implementation of the iterator and the code using it. I'm happy to write an RFC for that.

Praetonus · 2017-07-31T18:56:03Z

@plietar The problem with that is that every iterator-based loop would have to exit with an exception, which is very expensive.

I think caching would be the best solution here if performance turns out to be a problem.

jemc · 2017-07-31T18:59:29Z

@plietar - note that in cases where it's impossible to write a has_next, I have implemented it as statically true in the past - it's a bit of a code smell, but it still works in for loops, and is effectively the same as if you had removed it from Iterator as you suggest - it also has the same downsides that @Praetonus mentioned, though, and it could potentially be confusing to a user who was expecting to rely on has_next being true, but that's something that could perhaps be fixed with documentation.

SeanTAllen · 2017-08-02T19:44:37Z

This is sync approved with a tiny caveat.

@autodidaddict can you address @jemc's couple of small formatting issues and then we can merge this?

autodidaddict · 2017-08-02T19:54:33Z

I already addressed the formatting issues @SeanTAllen. It's in the "style guidelines" commit.

SeanTAllen · 2017-08-02T21:26:10Z

I added the missing '.' that was required for this to be merged.

SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Jul 30, 2017

Add regex match iterator

3e8c2d0

autodidaddict force-pushed the match-iter branch from b363cde to 3e8c2d0 Compare July 30, 2017 01:50

autodidaddict changed the title ~~Adding match iterator functionality from issue #2093, RFC 044~~ Add regex match iterator Jul 30, 2017

SeanTAllen reviewed Jul 30, 2017

View reviewed changes

autodidaddict added 2 commits July 29, 2017 22:05

Changing filename per conventions.

bfd4d28

Renaming test classes

3fc1913

jemc reviewed Jul 31, 2017

View reviewed changes

Updating per style guidelines.

af4487f

jemc approved these changes Jul 31, 2017

View reviewed changes

Add missing '.'

e08363e

SeanTAllen merged commit edb72c4 into ponylang:master Aug 2, 2017

ponylang-main added a commit that referenced this pull request Aug 2, 2017

Update CHANGELOG for PR #2109 [skip ci]

443d32b

SeanTAllen mentioned this pull request Aug 2, 2017

Last Week in Pony - August 6, 2017 ponylang/ponylang-website#120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regex match iterator #2109

Add regex match iterator #2109

autodidaddict commented Jul 30, 2017 •

edited

Loading

SeanTAllen Jul 30, 2017

SeanTAllen Jul 30, 2017 •

edited

Loading

SeanTAllen Jul 30, 2017

SeanTAllen commented Jul 30, 2017

jemc Jul 31, 2017

jemc Jul 31, 2017

autodidaddict Jul 31, 2017

jemc Jul 31, 2017

sylvanc Aug 2, 2017

jemc Jul 31, 2017

jemc commented Jul 31, 2017

plietar commented Jul 31, 2017

Praetonus commented Jul 31, 2017

jemc commented Jul 31, 2017

SeanTAllen commented Aug 2, 2017 •

edited

Loading

autodidaddict commented Aug 2, 2017 •

edited

Loading

SeanTAllen commented Aug 2, 2017

Add regex match iterator #2109

Add regex match iterator #2109

Conversation

autodidaddict commented Jul 30, 2017 • edited Loading

Choose a reason for hiding this comment

SeanTAllen Jul 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemc commented Jul 31, 2017

plietar commented Jul 31, 2017

Praetonus commented Jul 31, 2017

jemc commented Jul 31, 2017

SeanTAllen commented Aug 2, 2017 • edited Loading

autodidaddict commented Aug 2, 2017 • edited Loading

SeanTAllen commented Aug 2, 2017

autodidaddict commented Jul 30, 2017 •

edited

Loading

SeanTAllen Jul 30, 2017 •

edited

Loading

SeanTAllen commented Aug 2, 2017 •

edited

Loading

autodidaddict commented Aug 2, 2017 •

edited

Loading