POSIX: add Regex, update Regex to swift 3, API changes #168

bosr · 2016-09-29T06:59:15Z

Hi, this is not finished yet, but it's clear enough for you to begin reviewing

update to Swift 3
utf8 support
convenience operators ~, ~*, ~?
Tests for match and groups
Tests for replace
API change (needs review @paulofaria)
get rid of one Foundation dependency (used for one method)?

codecov-io · 2016-09-29T08:16:02Z

Current coverage is 82.99% (diff: 86.64%)

Merging #168 into master will increase coverage by 0.08%

@@             master       #168   diff @@
==========================================
  Files           179        181     +2   
  Lines         13260      13552   +292   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          10995      11248   +253   
- Misses         2265       2304    +39   
  Partials          0          0

Powered by Codecov. Last update 97b989b...9de9b9d

paulofaria · 2016-09-29T18:26:09Z

I'm not at home now, so I'll review it properly when I get home. But regarding the POSIXRegex module. I think it's better to add Regex to the existing POSIX module instead of reviving POSIXRegex from the graveyard.

bosr · 2016-09-29T18:43:20Z

Ok, i'll begin working on that.

bosr · 2016-09-29T22:27:56Z

@paulofaria I'm ready for the API change you suggest!
(others are welcome too :))

bosr · 2016-09-30T08:03:57Z

There is a strange thing with xcpretty (xcpretty/xcpretty#241) that does not help seeing the tests results for the XCode build part (the swiftpm part passes).

bosr · 2016-09-30T09:31:53Z

Modules/POSIX/Sources/POSIX/Regex.swift

+            var replacedStringArray = [UInt8](string.utf8)
+            replacedStringArray.replaceSubrange(start..<end, with: templateArray)
+
+            guard let replacedString = String(data: Data(replacedStringArray), encoding: .utf8) else {


This is the reason why Foundation is used. Should I try to find an alternative?

It's usually more performant to use String(cString:) see if you can use it.

paulofaria

This looks great! Thanks again! 😊

paulofaria · 2016-09-30T20:20:39Z

Modules/POSIX/Sources/POSIX/Regex.swift

+        var preg = preg
+        var buffer = [Int8](repeating: 0, count: Int(BUFSIZ))
+        regerror(result, &preg, &buffer, buffer.count)
+        let description = String(validatingUTF8: buffer)!


I think this one be String(cString: buffer).

paulofaria · 2016-09-30T20:26:58Z

Modules/POSIX/Sources/POSIX/Regex.swift

+    let description: String
+
+    static func error(from result: Int32, preg: regex_t) -> RegexError {
+        var preg = preg


The signature could be:

static func error(from result: Int32, preg: inout regex_t) -> RegexError

and then that copy:

var preg = preg

wouldn't be needed anymore.

paulofaria · 2016-09-30T20:28:36Z

Modules/POSIX/Sources/POSIX/Regex.swift

+
+public final class Regex {
+
+    public struct RegexOptions : OptionSet {


I think we can rename RegexOptions to just Options.

paulofaria · 2016-09-30T20:29:34Z

Modules/POSIX/Sources/POSIX/Regex.swift

+        }
+
+        public static let FirstCharacterNotAtBeginningOfLine = MatchOptions(rawValue: REG_NOTBOL)
+        public static let LastCharacterNotAtEndOfLine        = MatchOptions(rawValue: REG_NOTEOL)


These have to start with lowercase letters:

firstCharacterNotAtBeginningOfLine lastCharacterNotAtEndOfLine

paulofaria · 2016-09-30T20:35:13Z

Modules/POSIX/Sources/POSIX/Regex.swift

+
+        if result != 0 {
+            throw RegexError.error(from: result, preg: preg)
+        }


this is better expressed with a guard

guard result == 0 else { throw RegexError.error(from: result, preg: preg) }

If you change the signature of error(from:preg:) it will be:

guard result == 0 else { throw RegexError.error(from: result, preg: &preg) }

paulofaria · 2016-09-30T20:46:51Z

Modules/POSIX/Sources/POSIX/Regex.swift

+
+            if regexMatches[0].rm_eo == regexMatches[0].rm_so {
+                break // matches the empty string: infinite loop
+            }


ahah, incredible I missed all those guard statements, especially as I love them.

paulofaria · 2016-09-30T20:47:38Z

Modules/POSIX/Sources/POSIX/Regex.swift

+            var replacedStringArray = [UInt8](string.utf8)
+            replacedStringArray.replaceSubrange(start..<end, with: templateArray)
+
+            guard let replacedString = String(data: Data(replacedStringArray), encoding: .utf8) else {


It's usually more performant to use String(cString:) see if you can use it.

paulofaria · 2016-09-30T20:52:10Z

Modules/POSIX/Sources/POSIX/Regex.swift

+        return utf8view.index(utf8view.startIndex, offsetBy: Int(offset))
+    }
+
+    public func matches(in string: String, options: MatchOptions = []) -> Bool {


matches it's used as verb here like "a matches b". So the in label makes matches look like a noun like "the regex matches found in the string", and that's not what we mean. So we can remove the in label.

paulofaria · 2016-09-30T20:59:20Z

Modules/POSIX/Tests/POSIXTests/RegexTests.swift

+    func testReplaceUTF8() throws {
+        let r0 = try Regex(pattern: "coffee")
+        let actual0 = r0.replace(in: "Paulo loves coffee", with: "☕️")
+        XCTAssertEqual(actual0, "Paulo loves ☕️")


hahahahaha awesome! Paulo loves 🍻!

I could easily use String(bytes:encoding:) but not String(cString:) because it needs a CChar and I'd rather avoid an unsafeBitCast. Do you agree?

I added a second test regarding your comment on RegexTests :)

paulofaria · 2016-09-30T21:09:24Z

Modules/POSIX/Sources/POSIX/Regex.swift

+        return groups
+    }
+
+    public func replace(in string: String, with template: String, options: MatchOptions = []) -> String {


let's move around the parameters so it reads better.

public func replace(with template: String, in string: String, options: MatchOptions = []) -> String

supermarin · 2016-10-01T02:06:42Z

@bosr sorry about that - it went in on accident in one of the recent PRs. Fix is incoming in 0.2.4

bosr · 2016-10-01T06:18:56Z

@supermarin no worries, thanks for the update!

bosr · 2016-10-01T13:57:18Z

@paulofaria: updated and rebased

Cheers!

paulofaria

Added some ideas about moving the functions to String extensions, let me know what you think.

paulofaria · 2016-10-01T16:10:51Z

Modules/POSIX/Sources/POSIX/Regex.swift

+}
+
+// FIXME: let's talk about this
+//func ~* <Result> (left: String, right: String) throws -> ((String) -> Result) -> [Result] {


can you show me usage of this? 😊

Yep, it's just syntactic sugar for map or closure:

For the operators, I intend to adopt the ruby operator syntax =~

try ("hello world" =~ "(hello) (world)") { match in return match.contains("wo") ? match.uppercased() : match // Result = String }

or

try ("hello world" =~ "(hello) world") { match in print(match) // Result inferred to be Void }

but I now think that adding .map does not hurt :)

BTW, I tried to have this working without the parens but I could not find a good precedence group : the expressions above, when removing the parens, are understood as "the closure is applied to the 2nd string", which fails miserably :)

paulofaria · 2016-10-01T16:11:01Z

Modules/POSIX/Sources/POSIX/Regex.swift

+    /// - parameter utf8view: UTF8View of the string.
+    ///
+    /// - returns: UTF8View.Index of the character designated by `regoff`.
+    func index(from offset: regoff_t, in utf8view: String.UTF8View) -> String.UTF8View.Index {


Is this meant to be used in other places inside POSIX? If not, marking it private could help the optimizer.

yep, good idea

paulofaria · 2016-10-01T16:11:35Z

Modules/POSIX/Tests/POSIXTests/RegexTests.swift

+
+        let r1 = try Regex(pattern: "☕️")
+        let actual1 = r1.replace(with: "🍻", in: actual0)
+        let expected1 = "Paulo loves 🍻"


hahahah awesome!

paulofaria · 2016-10-01T16:13:18Z

Modules/POSIX/Sources/POSIX/Regex.swift

+
+            totalReplacedString += String(describing: replacedString.utf8[replacedString.utf8.startIndex ..< templateDeltaIndex])
+            let startIndex = string.utf8.index(string.utf8.startIndex, offsetBy: end)
+            string = String(describing: string.utf8[startIndex ..< string.utf8.endIndex])


String(describing:) will call the description property in CustomStringConvertible. I think you can just remove the describing: label and use the cast style initializer String(thing). This happens in other places too.

Was this the place you were using Foundation? I can't find it anymore, but the Foundation import is still up there.

You mentioned unsafe bit cast in the other review. If it's between Int8 and UInt8 there's no problem. But if you have a pointer you can just use withMemoryRebound or assuminMemoryBound depending on what kind of pointer you have. This way you can use the faster String(cString:). Having said that I think we can leave optimizations for another PR as long as we have correctness in this one. 😊

Hi, we cannot remove the describing: label, XCode insists that we should use it.

Regarding the use of Foundation, it is needed by

guard let replacedString = String(bytes: replacedStringArray, encoding: .utf8) else { break }

I do agree that these optimizations could be postponed to another PR :)

paulofaria · 2016-10-01T16:34:00Z

Modules/POSIX/Sources/POSIX/Regex.swift

+    public typealias ExtendedGraphemeClusterLiteralType = StringLiteralType
+
+    public convenience init(unicodeScalarLiteral value: UnicodeScalarLiteralType) {
+        try! self.init(pattern: value, options: .extended)


It sucks that literals can't throw maybe we can add this but put a giant warning on the docs?

paulofaria · 2016-10-01T16:43:42Z

Modules/POSIX/Sources/POSIX/Regex.swift

+    /// - parameter options: Options such as `Regex.Options.basic` (BRE) or `Regex.Options.extended` (ERE).
+    ///
+    /// - throws: `RegexError` if the given pattern is not a valid Regex.
+    public init(pattern: String, options: Options = .extended) throws {


I think we can remove the pattern label. It's pretty obvious the String is the pattern.

paulofaria · 2016-10-01T16:52:08Z

Modules/POSIX/Sources/POSIX/Regex.swift

+    /// - parameter options: Matching options like `firstCharacterNotAtBeginningOfLine` (`REG_NOTBOL` in POSIX parlance).
+    ///
+    /// - returns: `true` if the string matches the regex, else `false`.
+    public func matches(_ string: String, options: MatchOptions = []) -> Bool {


Just had an idea. What do you think about taking the public methods out of the Regex type and move them to a String extension receiving the Regex type as a parameter? This way the call site would become like this:

let regex = try Regex("hello") "hello".matches(regex) // true

let regex = try Regex("(hello) (world)") "hello world".groupsMatching(regex) // ["hello", "world"]

let regex = try Regex("world") "hello world".replace(regex, with: "zewo") // "hello zewo"

Yeah, thanks. I thought about this, but I first went for operators (discoverability a bit low anyway in both extensions and operators).

I'll do it soon

Danappelxx · 2016-10-02T21:39:05Z

Modules/POSIX/Sources/POSIX/Regex.swift

@@ -0,0 +1,319 @@
+#if os(Linux)
+    @_exported import Glibc


I believe we don't want to export the libc api - just import it locally.

nice catch!

oops, thanks

bosr · 2016-10-03T09:43:32Z

I will update soon, thanks for the review 👍

bosr · 2016-10-07T06:14:12Z

Hi, I updated according to the review requests (sorry I could not come back to you any sooner).

I'll try tonight to see if rebasing does not break it :)

Cheers

…ules/POSIXRegex subrepo: subdir: "Modules/POSIXRegex" merged: "67f3d04" upstream: origin: "https://github.com/ZewoGraveyard/POSIXRegex.git" branch: "master" commit: "67f3d04" git-subrepo: version: "0.3.0" origin: "???" commit: "???"

bosr · 2016-10-13T11:39:39Z

Just rebased

bosr force-pushed the feature/posix-regex branch from 940e5fa to 22e043f Compare September 29, 2016 08:16

bosr force-pushed the feature/posix-regex branch from 22e043f to e7892e7 Compare September 29, 2016 08:17

bosr mentioned this pull request Sep 29, 2016

[POSIX] Add Regex to POSIX #132

Closed

2 tasks

bosr force-pushed the feature/posix-regex branch from 87b2ff9 to 6331e04 Compare September 29, 2016 22:10

bosr commented Sep 30, 2016

View reviewed changes

bosr changed the title ~~POSIX Regex update to swift 3 + API changes~~ POSIX: add Regex, update Regex to swift 3, API changes Sep 30, 2016

paulofaria reviewed Sep 30, 2016

View reviewed changes

bosr force-pushed the feature/posix-regex branch from 6331e04 to e81b2ed Compare October 1, 2016 07:02

paulofaria reviewed Oct 1, 2016

View reviewed changes

Danappelxx reviewed Oct 2, 2016

View reviewed changes

Romain Bossart added 8 commits October 13, 2016 13:34

conform POSIXRegex to Zewo/Zewo architecture

f6778d1

utf8 regex and operators

1963746

move Regex to POSIX module

c918463

add replace tests

d5fa0fd

[review] guards and signature changes

be66377

add documentation

dab3931

review-2: remove @_exported, use string extension, ...

9de9b9d

bosr force-pushed the feature/posix-regex branch from 933377a to 9de9b9d Compare October 13, 2016 11:39

paulofaria merged commit 1b67b53 into Zewo:master Oct 13, 2016

bosr deleted the feature/posix-regex branch October 24, 2016 10:13


		public final class Regex {

		public struct RegexOptions : OptionSet {

POSIX: add Regex, update Regex to swift 3, API changes #168

POSIX: add Regex, update Regex to swift 3, API changes #168

Conversation

bosr commented Sep 29, 2016 • edited

codecov-io commented Sep 29, 2016 • edited

Current coverage is 82.99% (diff: 86.64%)

paulofaria commented Sep 29, 2016

bosr commented Sep 29, 2016 • edited

bosr commented Sep 29, 2016 • edited

bosr commented Sep 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulofaria left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulofaria Sep 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulofaria Sep 30, 2016 • edited

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

bosr Oct 1, 2016 • edited

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

supermarin commented Oct 1, 2016

bosr commented Oct 1, 2016 • edited

bosr commented Oct 1, 2016

paulofaria left a 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

bosr Oct 3, 2016 • edited

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

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

bosr Oct 3, 2016 • edited

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

bosr commented Oct 3, 2016 • edited

bosr commented Oct 7, 2016

bosr commented Oct 13, 2016

bosr commented Sep 29, 2016 •

edited

codecov-io commented Sep 29, 2016 •

edited

bosr commented Sep 29, 2016 •

edited

bosr commented Sep 29, 2016 •

edited

bosr commented Sep 30, 2016 •

edited

paulofaria Sep 30, 2016 •

edited

paulofaria Sep 30, 2016 •

edited

bosr Oct 1, 2016 •

edited

bosr commented Oct 1, 2016 •

edited

bosr Oct 3, 2016 •

edited

bosr Oct 3, 2016 •

edited

bosr commented Oct 3, 2016 •

edited