[SE-0046] Implementation for consistent func labels #2047

manavgabhawala · 2016-04-04T05:15:09Z

What's in this pull request?

Implements SE-0046 to provide consistent function labels (i.e. the first parameter no longer has special behavior for function labels).

I separated out the changes into separate commits depending on the part being changed so that its easier to review since this is a massive change across the entire repo. However, if it is preferred that this is merged as a single commit, I would be happy to squash into one commit.

I updated most of the swift code using Regex find and replace (and then manual verification where necessary) and since a lot of other repos are going to suffer source code breaking changes to make it easier for them, here are the regex strings I used:
For regular (non-generic) functions I did a search for func ([_a-zA-Z0-9]+?)(\s*?)\((\s*?)([a-zA-Z0-9]+?)(\s*?): and replaced with func \1\2(\3_ \4\5: By estimation this worked about 95% of the time and almost never found any false positives.
For generic functions I did a second search using: func ([_a-zA-Z0-9]+?)(\s*?)(<[\s\S]*?>)(\s*?)\((\s*?)([a-zA-Z0-9]+?)(\s*?): and replaced with func \1\2\3\4(\5_ \6\7: This worked about 90% of the time but be warned it does tend to find false positives (rarely).

Resolved bug number:

SR-961
rdar://problem/25279084

Before merging this pull request to apple/swift repository:

Review changes
Other swift repositories should be ready for a source code breaking change
Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

gribozavr · 2016-04-04T10:11:28Z

stdlib/private/StdlibCollectionUnittest/LoggingWrappers.swift.gyb

    C : Collection where C.Iterator.Element == Base.Iterator.Element
  >(
-    bounds: Range<Base.Index>, with newElements: C
+    subRange bounds: Range<Base.Index>, with newElements: C


Please don't change the standard library API. It was already audited and it conforms to the guidelines.

gribozavr · 2016-04-04T10:13:17Z

CC @harlanhaskins

gribozavr · 2016-04-04T10:14:47Z

@manavgabhawala Thank you! I am extremely concerned about false positives. 95% is good, but not perfect. We can't allow any unintended changes in the standard library API. Did you manually audit all of the changes to the stdlib directory?

harlanhaskins · 2016-04-04T14:09:53Z

@manavgabhawala Great job! I definitely share @gribozavr's concerns about API -- it's tedious but you'll need to manually verify your regex didn't cause any unintended API changes.

manavgabhawala · 2016-04-04T15:20:32Z

@gribozavr @harlanhaskins I did not use the regex for any of the stdlib changes for exactly that reason. I did not change any of the APIs other than the replaceSubrange function (and not just in String, in all the containers), is because almost all of the declarations were of the form replaceSubrange(_ subRange:,with:) so I thought this might be an easier change to do. However if you want, I can go ahead and change all of these back to the original form.

harlanhaskins · 2016-04-04T17:29:39Z

I think it's probably best to change it back for this pull request.

harlanhaskins · 2016-04-04T17:53:21Z

There are some conflict markers scattered throughout stdlib/core/Reflection.swift

manavgabhawala · 2016-04-04T19:35:32Z

@harlanhaskins will fix, thanks for the catch

manavgabhawala · 2016-04-04T23:29:38Z

@harlanhaskins fixed all the changes requested

harlanhaskins · 2016-04-04T23:33:04Z

Were those conflicts from abfecfd? Thanks for fixing those so quickly!

I'm gonna take another look over the changes to make sure nothing stands out.

manavgabhawala · 2016-04-04T23:38:38Z

No I think they were from 9ce9784180be8ebc7744a7a295c6006f543da344. I had fixed them but I think I forgot to save the file before I rebased. But it should be fixed now. There are always conflicts every time there are new commits since this PR changes a lot of files.

Yeah absolutely, please make sure nothing stands out.

harlanhaskins · 2016-04-04T23:51:48Z

We're also going to need to have branches ready to go in SPM, corelibs, lldb, and xctest, because as soon as Top-of-Tree swiftc receives this change, all subprojects will fail to build.

I'll coordinate with @mxcl, @parkera, @ddunbar, @modocache, and @eschaton to get their projects ready.

manavgabhawala · 2016-04-04T23:54:12Z

Great, sounds good. Let me know if there's anything I can help with

harlanhaskins · 2016-04-05T00:01:43Z

stdlib/public/core/ArrayBufferProtocol.swift

@@ -69,8 +69,8 @@ public protocol _ArrayBufferProtocol : MutableCollection {
  ///
  /// - Precondition: This buffer is backed by a uniquely-referenced
  /// `_ContiguousArrayBuffer`.
-  mutating func replace<C : Collection where C.Iterator.Element == Element>(
-    subRange subRange: Range<Int>,
+  mutating func replaceSubrange<C : Collection where C.Iterator.Element == Element>(


Lingering API change here

harlanhaskins · 2016-04-05T00:27:46Z

I can't see the file right now in GitHub's diff view (too many changes 😂), but whilst running tests I found a conflict marker in test/expr/unary/selector/selector.swift

harlanhaskins · 2016-04-05T00:41:55Z

stdlib/public/core/ArrayBuffer.swift

@@ -392,8 +391,8 @@ extension _ArrayBuffer {
      }
      else {
        var refCopy = self
-        refCopy.replace(
-          subRange: i...i, with: 1, elementsOf: CollectionOfOne(newValue))
+        refCopy.replaceSubrange(


API change here

harlanhaskins · 2016-04-06T00:40:36Z

@manavgabhawala Also it looks like the extra diagnostics didn't get deleted from doc_swift_module.swift.response.

gribozavr · 2016-04-06T01:00:07Z

@harlanhaskins I know. I hoped to get at least some CI feedback.

gribozavr · 2016-04-06T01:00:35Z

And we have some:

/Users/buildnode/jenkins/workspace/swift-PR-osx/swift/stdlib/public/core/ObjCMirrors.swift:46:41: error: missing argument label 'object:' in call
    return _getClassPlaygroundQuickLook(object)

harlanhaskins · 2016-04-06T01:02:17Z

Interesting! I didn't get that one when I built with --test --validation-test...

harlanhaskins · 2016-04-06T01:06:28Z

Oh, these are new changes from master?

manavgabhawala · 2016-04-06T03:11:50Z

@harlanhaskins can I rebase now, if you are done reviewing? so that I can fixup the new extra commits that I pushed and also get all the new changes to master?

harlanhaskins · 2016-04-06T03:15:09Z

Yes, absolutely! Thanks again!

We're going to coordinate this with corelibs-foundation, XCTest, lldb, and SPM, to get all of these merged together at the same time.

There will undoubtedly be at least one more rebase in your future.

shahmishal · 2016-04-06T21:35:10Z

@swift-ci Please test

shahmishal · 2016-04-06T21:36:09Z

@swift-ci Please smoke test

harlanhaskins · 2016-04-06T22:50:43Z

Smoke test passed. Are we set to merge? @gribozavr

We'll need to coordinate with swiftlang/swift-corelibs-xctest#92 and swiftlang/swift-corelibs-foundation#305

harlanhaskins · 2016-04-06T22:59:00Z

Some conflicts just arose 😕

@manavgabhawala Mind fixing these and squashing into one commit?

harlanhaskins · 2016-04-06T23:17:18Z

You just need to delete the definitions for insetBy and the like, inside stdlib/core/SDK/CoreGraphics/CoreGraphics.swift.

manavgabhawala · 2016-04-07T00:15:49Z

@harlanhaskins do you want just one commit total, or squash into the current 5, correctly?

harlanhaskins · 2016-04-07T00:16:10Z

@manavgabhawala One commit total, if possible.

…ng extraneous parameter names and adding _ where necessary

manavgabhawala · 2016-04-07T00:25:51Z

@harlanhaskins done. Should we re- smoke test?

harlanhaskins · 2016-04-07T00:26:34Z

@manavgabhawala I don't think that will be necessary. @gribozavr?

harlanhaskins · 2016-04-07T00:40:54Z

Thank you, @manavgabhawala!

Merge main 2020-10-21

manavgabhawala force-pushed the consistent-func-labels branch from ac9d274 to 7cae734 Compare April 4, 2016 07:21

gribozavr reviewed Apr 4, 2016
View reviewed changes

manavgabhawala force-pushed the consistent-func-labels branch 3 times, most recently from ed2f09a to 6207d38 Compare April 4, 2016 23:24

harlanhaskins reviewed Apr 5, 2016
View reviewed changes

manavgabhawala force-pushed the consistent-func-labels branch from 906c6fd to a6d2029 Compare April 6, 2016 04:24

briancroom mentioned this pull request Apr 6, 2016

[SE-0046] Implemented consistent function labels swiftlang/swift-corelibs-xctest#91

Closed

This was referenced Apr 6, 2016

[SE-0046] Implemented consistent function labels swiftlang/swift-package-manager#242

Merged

[SE-0046] Implemented consistent function labels apple/swift-lldb#8

Merged

harlanhaskins mentioned this pull request Apr 6, 2016

[SE-0046] Implemented consistent function labels swiftlang/swift-corelibs-xctest#92

Merged

[SE-0046] Implements consistent function parameter labels by discardi…

7928140

…ng extraneous parameter names and adding _ where necessary

manavgabhawala force-pushed the consistent-func-labels branch from a6d2029 to 7928140 Compare April 7, 2016 00:22

harlanhaskins merged commit 510f29a into swiftlang:master Apr 7, 2016

harlanhaskins mentioned this pull request Apr 7, 2016

Allow subclasses of specific instantiations of ObjC generic classes. #2086

Merged

manavgabhawala deleted the consistent-func-labels branch April 8, 2016 23:20

manavgabhawala mentioned this pull request Apr 9, 2016

Update README to move SE-0046 to implemented swiftlang/swift-evolution#250

Merged

MaxDesiatov pushed a commit that referenced this pull request Apr 19, 2021

Merge pull request #2047 from swiftwasm/katei/merge-main-2020-10-21

be3dce1

Merge main 2020-10-21

swift-ci mentioned this pull request Apr 4, 2016

[SR-961] Implement SE-0046: Establish consistent label behavior across all parameters including first labels #43573

Closed

[SE-0046] Implementation for consistent func labels #2047

[SE-0046] Implementation for consistent func labels #2047

Uh oh!

Conversation

manavgabhawala commented Apr 4, 2016

What's in this pull request?

Resolved bug number:

Uh oh!

gribozavr Apr 4, 2016

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Apr 4, 2016

Uh oh!

gribozavr commented Apr 4, 2016

Uh oh!

harlanhaskins commented Apr 4, 2016

Uh oh!

manavgabhawala commented Apr 4, 2016

Uh oh!

harlanhaskins commented Apr 4, 2016

Uh oh!

harlanhaskins commented Apr 4, 2016

Uh oh!

manavgabhawala commented Apr 4, 2016

Uh oh!

manavgabhawala commented Apr 4, 2016

Uh oh!

harlanhaskins commented Apr 4, 2016

Uh oh!

manavgabhawala commented Apr 4, 2016

Uh oh!

harlanhaskins commented Apr 4, 2016

Uh oh!

manavgabhawala commented Apr 4, 2016

Uh oh!

harlanhaskins Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

harlanhaskins commented Apr 5, 2016

Uh oh!

harlanhaskins Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

gribozavr commented Apr 6, 2016

Uh oh!

gribozavr commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

manavgabhawala commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

shahmishal commented Apr 6, 2016

Uh oh!

shahmishal commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

manavgabhawala commented Apr 7, 2016

Uh oh!

harlanhaskins commented Apr 7, 2016

Uh oh!

manavgabhawala commented Apr 7, 2016

Uh oh!

harlanhaskins commented Apr 7, 2016

Uh oh!

harlanhaskins commented Apr 7, 2016

Uh oh!

Uh oh!