Implement NSTextCheckingResult.range(withName:) #1522

nevil · 2018-04-18T15:51:45Z

Added the missing range(withName:) and corresponding CF function.
The implementation relies on uregex_groupNumberFromName which is
available as a draft API from ICU 55.
Added tests related to named capture groups.

spevans · 2018-04-18T15:54:14Z

@swift-ci please test

nevil · 2018-04-18T23:32:02Z

I realized that I'm not sure what to do with the availability story.
Should I do the following:

add API_AVAILABLE(macosx(10.13), ios(11.0), watchos(4.0), tvos(11.0)) to the CF API
add @available(macOS 10.13, iOS 11.0, watchOS 4.0, tvOS 11.0, *) to the NSTextCheckingResult API
Condition the unit tests based on availability?

parkera · 2018-04-22T02:56:31Z

@swift-ci please test

nevil · 2018-04-23T23:30:39Z

@parkera Seems like the test wasn't run or the result not reported back?

spevans · 2018-04-24T09:10:48Z

@swift-ci please test

nevil · 2018-04-24T12:58:50Z

The build passed but the from log it seems it was run on a commit that was squashed and no longer exist (0aedad0) even before @spevans requested the build ...
https://ci.swift.org/job/swift-corelibs-foundation-PR-Linux/918/

nevil · 2018-04-26T02:16:30Z

Would a kind soul please rerun the test? :-)

spevans · 2018-04-26T07:05:48Z

@swift-ci please test

nevil · 2018-05-07T02:08:00Z

@parkera
Is there anything more I should do for this PR?

Not sure what the normal review time frames are...

millenomi

Just one nitpick and then it’s good to merge:

millenomi · 2018-05-07T03:57:32Z

CoreFoundation/String.subproj/CFRegularExpression.h

@@ -57,6 +57,7 @@ _CFRegularExpressionRef _Nullable _CFRegularExpressionCreate(CFAllocatorRef allo
 void _CFRegularExpressionDestroy(_CFRegularExpressionRef regex);

 CFIndex _CFRegularExpressionGetNumberOfCaptureGroups(_CFRegularExpressionRef regex);
+CFIndex _CFRegularExpressionGetCaptureGroupNumberWithName(_CFRegularExpressionRef regex, CFStringRef groupName) API_AVAILABLE(macosx(10.13), ios(11.0), watchos(4.0), tvos(11.0));


Why availability annotations? Internal calls do not need them, and in this case they would also be inaccurate (this call isn’t on any of the mentioned OSes.)

Just remove them, here and in the Swift code?

Thanks @millenomi !

I wasn't sure what to do with the annotations so to be safe I made sure to add to all APIs.
Sorry for that.

I removed on internal APIs and only left on the public NSTextCheckingResult.range(withName:).

spevans · 2018-05-07T09:03:56Z

@swift-ci please test

nevil · 2018-05-07T09:10:26Z

@spevans Thanks for starting the build!
Unfortunately it really looks like the swift CI infrastructure does not like git rebases...
The current commit is 4b22b29 but the build is running on e167c45, which was the commit before the rebase...

https://ci.swift.org/view/Pull%20Request/job/swift-corelibs-foundation-PR-Linux/961/console

spevans · 2018-05-07T09:14:10Z

There is just one issue I see with this change and its that Ubuntu 14.04 uses ICU 52 and so doesn't have the uregex_groupNumberFromName function from what I can see (checking the libs on my 14.04 install).

Is it possible to use the U_ICU_VERSION_MAJOR_NUM (defined as 52 on 14.04 and 55 on 16.04) to mark this function as unavailable on the older version (or maybe there is an alternative solution).

I think this will cause problems if merged as it will then break the 14.04 package build (although it is current broken due to a different 14.04/ ICU issue anyway, see #1537)

spevans · 2018-05-07T09:18:50Z

@nevil swift-ci looks fine, its just misleading with the first comment.
There is a log entry that says

Commit message: "Merge 4b22b2929e253bf50564498cf4e208443e8559d0 into 55bb988867eb5c8e174d67a0f91a15d9efdd072a"

which means its merging in the correct version.
Rebases and force pushes work fine, I use them all the time without issue.

nevil · 2018-05-07T09:41:32Z

@spevans You're correct. Looks like the commit hash mismatch is just for the GitHub build status reporting...

I could of course use U_ICU_VERSION_MAJOR_NUM (or some other flag) to conditionally enable the _CFRegularExpressionGetCaptureGroupNumberWithName, but the question is if this is an acceptable workaround until all supported Linux versions use the same ICU library?

It would introduce a version difference that can not be checked through the use of availability annotation checks...

spevans · 2018-05-07T09:58:35Z

You could do something like the following:

Define HAS_ICU_GROUP_NUMBER_FROM_NAME based on the ICU version and conditionally compile the support functions, then add:

#if !HAS_ICU_GROUP_NUMBER_FROM_NAME
@available(*, unavailable, message: "Unuspported with this version of ICU")
override func range(withName name: String) -> NSRange {
  NSUnsupported()
}
#else
override func range(withName name: String) -> NSRange {
.
.
.
}
#endif

millenomi · 2018-05-25T23:42:55Z

@nevil What do you think about @spevans's suggestion?

nevil · 2018-05-26T07:15:44Z

@millenomi
I will try to implement something like what he suggested.
Unfortunately I haven't had time to study the build system until now.

millenomi · 2018-10-11T03:24:52Z

@nevil Are you still interested in chaperoning this patch?

nevil · 2018-10-11T08:09:45Z

@millenomi
My intention was to complete the task ASAP back in May, but I had the problem that my local machine doesn't have enough RAM to do a complete linux build in a VM. I thought I would have access to a better machine soon, but didn't and then time passed quickly...

Sorry for that!
I still have an interest and would like to complete the PR.

Is it best to try to get this in with build time version detection of ICU (as suggested here
#1522 (comment)) or wait for

https://forums.swift.org/t/building-libicu-as-part-of-the-linux-swift-build/16526/8 to settle?
https://bugs.swift.org/browse/SR-8876 .

spevans · 2018-10-11T09:36:48Z

@nevil The PR to always build the latest version of ICU for linux should hopefully be ready in the next day or so - I just have to fix one LLDB test - so you can probably leave the version detection out now. In fact it will be good to since it should catch builds using the old version since the symbols won't be defined.

nevil · 2018-10-11T09:43:06Z

@spevans Great. Thank you very much!

I will rebase this PR on the latest master branch in preparation for your PR to be merged.

nevil · 2018-10-11T14:04:39Z

Nothing had really changed in the touched files so the rebase was straight forward.

nevil · 2018-10-20T01:57:26Z

Waiting for swiftlang/swift#19860

nevil · 2018-11-05T02:00:57Z

@spevans
Now that swiftlang/swift#19860 is merged, my understanding is that we will always use a compatible ICU version.
Is there anything build related I should consider in this PR?

spevans · 2018-11-05T17:46:39Z

@swift-ci test

nevil · 2018-11-06T13:56:59Z

@millenomi
I think the build passed.
Please review again when you have time.

Thanks!

- Added the missing range(withName:) and corresponding CF function. The implementation relies on uregex_groupNumberFromName which is available as a draft API from ICU 55. - Added tests related to named capture groups. - Add availability checks in the tests to run with DarwinCompatibilityTests.

nevil · 2019-02-12T01:53:48Z

@millenomi
Hi, I understand that you are busy with Swift 5.
Just read the latest https://nshipster.com/swift-regular-expressions/ and noticed that named capture groups are mentioned...

Any chance of reviewing again?

spevans · 2019-02-12T07:34:01Z

@swift-ci please test

parkera · 2019-02-13T18:20:29Z

This looks good to me.

parkera · 2019-02-13T18:20:56Z

Sorry this took so long and thank you for the contribution!

nevil · 2019-02-13T22:24:02Z

@parkera
We all have lots of things to do :-)
Thank you for merging!

motivation: * netrc functionality is useful on all platforms, the regex functionality was added to non-darwin foundation * see swiftlang/swift-corelibs-foundation#1522 changes: remove platforms restructions where appropriate

nevil force-pushed the add-range-withname-support branch from 0aedad0 to e167c45 Compare April 23, 2018 23:30

This was referenced Apr 24, 2018

Add file name matching to file_header rule realm/SwiftLint#2166

Merged

File Header Rule - Check Filename realm/SwiftLint#1079

Closed

millenomi reviewed May 7, 2018

View reviewed changes

nevil force-pushed the add-range-withname-support branch from e167c45 to 4b22b29 Compare May 7, 2018 07:19

nevil force-pushed the add-range-withname-support branch from 4b22b29 to bb56345 Compare May 25, 2018 10:20

spevans mentioned this pull request Jun 9, 2018

Add support for .withFractionalSeconds to ISO8601DateFormatter. #1586

Merged

nevil force-pushed the add-range-withname-support branch from bb56345 to d8ed8a6 Compare October 11, 2018 13:58

nevil force-pushed the add-range-withname-support branch from d8ed8a6 to 2772e1a Compare November 5, 2018 01:59

nevil force-pushed the add-range-withname-support branch from 2772e1a to 65319df Compare November 26, 2018 11:14

nevil force-pushed the add-range-withname-support branch from 65319df to 34f3ce5 Compare December 8, 2018 01:44

nevil force-pushed the add-range-withname-support branch from 34f3ce5 to 8ce01f2 Compare January 17, 2019 07:11

nevil force-pushed the add-range-withname-support branch from 8ce01f2 to 35739c9 Compare February 12, 2019 01:50

parkera approved these changes Feb 13, 2019

View reviewed changes

parkera merged commit 531cca6 into swiftlang:master Feb 13, 2019

nevil deleted the add-range-withname-support branch February 13, 2019 22:24

tomerd mentioned this pull request May 3, 2021

enable netrc on non-darwin platforms swiftlang/swift-tools-support-core#210

Merged

tomerd mentioned this pull request May 4, 2021

enable netrc on non-darwin platforms swiftlang/swift-tools-support-core#211

Closed

Implement NSTextCheckingResult.range(withName:) #1522

Implement NSTextCheckingResult.range(withName:) #1522

Uh oh!

Conversation

nevil commented Apr 18, 2018

Uh oh!

spevans commented Apr 18, 2018

Uh oh!

nevil commented Apr 18, 2018

Uh oh!

parkera commented Apr 22, 2018

Uh oh!

nevil commented Apr 23, 2018

Uh oh!

spevans commented Apr 24, 2018

Uh oh!

nevil commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nevil commented Apr 26, 2018

Uh oh!

spevans commented Apr 26, 2018

Uh oh!

nevil commented May 7, 2018

Uh oh!

millenomi left a comment

Choose a reason for hiding this comment

Uh oh!

millenomi May 7, 2018

Choose a reason for hiding this comment

Uh oh!

nevil May 7, 2018

Choose a reason for hiding this comment

Uh oh!

spevans commented May 7, 2018

Uh oh!

nevil commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spevans commented May 7, 2018

Uh oh!

spevans commented May 7, 2018

Uh oh!

nevil commented May 7, 2018

Uh oh!

spevans commented May 7, 2018

Uh oh!

millenomi commented May 25, 2018

Uh oh!

nevil commented May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

millenomi commented Oct 11, 2018

Uh oh!

nevil commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spevans commented Oct 11, 2018

Uh oh!

nevil commented Oct 11, 2018

Uh oh!

nevil commented Oct 11, 2018

Uh oh!

nevil commented Oct 20, 2018

Uh oh!

nevil commented Nov 5, 2018

Uh oh!

spevans commented Nov 5, 2018

Uh oh!

nevil commented Nov 6, 2018

Uh oh!

nevil commented Feb 12, 2019

Uh oh!

spevans commented Feb 12, 2019

Uh oh!

parkera commented Feb 13, 2019

Uh oh!

parkera commented Feb 13, 2019

Uh oh!

nevil commented Feb 13, 2019

nevil commented Apr 24, 2018 •

edited

Loading

nevil commented May 7, 2018 •

edited

Loading

nevil commented May 26, 2018 •

edited

Loading

nevil commented Oct 11, 2018 •

edited

Loading