[stdlib][SE-0089] Finish off Lossless String Conversion #3761

CodaFi · 2016-07-26T07:53:04Z

What's in this pull request?

While Max is away, this patch finishes up his work on SE-0089.

Resolved bug number: (SR-1881)

Before merging this pull request to apple/swift repository:

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
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

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

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

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

CodaFi · 2016-07-26T07:54:26Z

stdlib/public/core/UnicodeScalar.swift

+extension UnicodeScalar : LosslessStringConvertible {
+  public init?(_ description: String) {
+    _precondition(description.unicodeScalars.count == 1)
+    if let v = description.unicodeScalars.first {


This feels too clever.

CodaFi · 2016-07-26T21:45:34Z

@swift-ci please test.

CodaFi · 2016-07-26T21:53:02Z

@gribozavr @dabrahams Mind a quick review?

gribozavr · 2016-07-26T21:56:30Z

stdlib/public/core/Character.swift

@@ -355,7 +355,7 @@ public struct Character :

 extension Character : CustomStringConvertible {
  public var description: String {
-    return String(self)
+    return String(describing: self)


This method should not go through reflection either, there are better, more performant ways to do this. (And wouldn't it call itself in fact?)

gribozavr · 2016-07-26T22:08:54Z

Reviewed.

CodaFi · 2016-07-27T19:03:16Z

@swift-ci please smoke test.

CodaFi · 2016-07-27T22:39:16Z

Patches are lined up for the other repos and tested locally - this should be ready for manual merge after we sort the bots.

CodaFi · 2016-07-28T18:37:19Z

@tkremenek Would you mind simultaneously merging this with

swiftlang/swift-package-manager#567
swiftlang/swift-corelibs-xctest#141
swiftlang/swift-corelibs-foundation#480

gribozavr · 2016-07-28T18:40:33Z

@CodaFi I merged more tests to test/1_stdlib/Test*.swift for Foundation types, that are very similar to tests that you had to adjust. Could you double-check that the new tests don't break with this PR?

CodaFi · 2016-07-28T18:54:24Z

Thanks for letting me know, testing now.

tkremenek · 2016-07-28T19:42:11Z

@swift-ci smoke test

CodaFi · 2016-07-28T21:15:40Z

@tkremenek Ready to merge.

gribozavr · 2016-07-28T21:20:09Z

@swift-ci Please test

gribozavr · 2016-07-28T23:01:53Z

@CodaFi Seems like we need fixes for the playgrounds repo:

/Users/buildnode/jenkins/workspace/swift-PR-osx/swift-xcode-playground-support/PlaygroundLogger/PlaygroundLogger/LoggerMirror.swift:122:16: error: cannot invoke initializer for type 'String' with an argument list of type '(Any)'

gribozavr · 2016-07-28T23:02:59Z

And Foundation, too:

Foundation/NSBundle.swift:66:19: error: cannot invoke initializer for type 'String' with an argument list of type '(Bundle.Type)'
        return "\(String(Bundle.self)) <\(bundleURL.path!)> (\(isLoaded  ? "loaded" : "not yet loaded"))"

shahmishal · 2016-07-29T00:43:17Z

We are seeing following test failures in CI (Ubuntu 15.10), after merging this PR.
https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-15_10/6928/console

Failing Tests (3):
    Swift(linux-x86_64) :: StdlibUnittest/RaceTest.swift
    Swift(linux-x86_64) :: stdlib/AtomicInt.swift
    Swift(linux-x86_64) :: stdlib/String.swift

CodaFi · 2016-07-29T00:50:45Z

So so sorry. Patch incoming.

CodaFi reviewed Jul 26, 2016
View reviewed changes

CodaFi force-pushed the lose-less-smile-more branch 5 times, most recently from 3b38691 to 5eeddf5 Compare July 26, 2016 21:43

CodaFi mentioned this pull request Jul 26, 2016

Minor update for SE-0089 swiftlang/swift-corelibs-foundation#480

Merged

gribozavr reviewed Jul 26, 2016
View reviewed changes

CodaFi force-pushed the lose-less-smile-more branch 6 times, most recently from 5629b9a to db8be58 Compare July 27, 2016 18:36

gribozavr mentioned this pull request Jul 27, 2016

[WIP] [stdlib] [SE-0089] Rename String.init(_:) #3574

Closed

1 task

CodaFi mentioned this pull request Jul 27, 2016

Updates for SE-0089 swiftlang/swift-corelibs-xctest#141

Merged

CodaFi mentioned this pull request Jul 27, 2016

Updates for SE-0089 swiftlang/swift-package-manager#567

Merged

frootloops and others added 2 commits July 28, 2016 11:49

Rename string reflection init

e5864b2

Addressing PR comments and updating some tests

ffe4eda

CodaFi force-pushed the lose-less-smile-more branch from db8be58 to 7c2b328 Compare July 28, 2016 18:49

Update test suite for lossless string conversion

ec9998f

CodaFi force-pushed the lose-less-smile-more branch from 7c2b328 to ec9998f Compare July 28, 2016 19:32

tkremenek merged commit 4c2dbe1 into swiftlang:master Jul 29, 2016

jessesquires mentioned this pull request Jul 29, 2016

Issue 33 - after Swift 3 SwiftWeekly/swiftweekly.github.io#57

Closed

CodaFi deleted the lose-less-smile-more branch December 22, 2019 02:59

CodaFi mentioned this pull request Jul 29, 2016

[SR-1881] Implement SE-0089: Renaming String.init<T>(_: T) #44490

Closed

[stdlib][SE-0089] Finish off Lossless String Conversion #3761

[stdlib][SE-0089] Finish off Lossless String Conversion #3761

Uh oh!

Conversation

CodaFi commented Jul 26, 2016

What's in this pull request?

Resolved bug number: (SR-1881)

Uh oh!

CodaFi Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jul 26, 2016

Uh oh!

CodaFi commented Jul 26, 2016

Uh oh!

gribozavr Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jul 26, 2016

Uh oh!

CodaFi commented Jul 27, 2016

Uh oh!

CodaFi commented Jul 27, 2016

Uh oh!

CodaFi commented Jul 28, 2016

Uh oh!

gribozavr commented Jul 28, 2016

Uh oh!

CodaFi commented Jul 28, 2016

Uh oh!

tkremenek commented Jul 28, 2016

Uh oh!

CodaFi commented Jul 28, 2016

Uh oh!

gribozavr commented Jul 28, 2016

Uh oh!

gribozavr commented Jul 28, 2016

Uh oh!

gribozavr commented Jul 28, 2016

Uh oh!

shahmishal commented Jul 29, 2016

Uh oh!

CodaFi commented Jul 29, 2016

Uh oh!

Uh oh!