[swift-evolution] [Review] SE-0027 Expose code unit initializers on String

Zach Waldowski zach at waldowski.me
Mon Feb 15 11:48:05 CST 2016 

See responses inline.

Aside: omgomgomg I got Dave Abrahams to respond to a proposal of mine

Excitedly,
  Zachary Waldowski
  zach at waldowski.me

On Mon, Feb 15, 2016, at 11:48 AM, Dave Abrahams via swift-evolution
wrote:
> 
> on Thu Feb 11 2016, Douglas Gregor <swift-evolution at swift.org> wrote:
> 
> > Hello Swift community,
> >
> > The review of SE-0027 "Expose code unit initializers on String" begins
> > now and runs through February 16, 2016. The proposal is available
> > here:
> >
> > https://github.com/apple/swift-evolution/blob/master/proposals/0027-string-from-code-units.md
> > Reviews are an important part of the Swift evolution process. All
> > reviews should be sent to the swift-evolution mailing list at
> >
> > https://lists.swift.org/mailman/listinfo/swift-evolution
> > <https://lists.swift.org/mailman/listinfo/swift-evolution>
> > or, if you would like to keep your feedback private, directly to the
> > review manager. When replying, please try to keep the proposal link at
> > the top of the message:
> >
> > Proposal link:
> >
> > https://github.com/apple/swift-evolution/blob/master/proposals/0027-string-from-code-units.md
> > Reply text
> >
> > Other replies
> >  <https://github.com/apple/swift-evolution#what-goes-into-a-review-1>What goes into a review?
> >
> > The goal of the review process is to improve the proposal under review
> > through constructive criticism and, eventually, determine the
> > direction of Swift. When writing your review, here are some questions
> > you might want to answer in your review:
> >
> > What is your evaluation of the proposal?
> 
> I support the intent of the proposal but am -1 on the specific proposal
> as offered.
> 
> First, in the presentation, there are several things that make it hard
> to evaluate:
> 
> 1. The "decode" function signature is so wide that it can't be read
>    without scrolling at
>    https://github.com/apple/swift-evolution/blob/master/proposals/0027-string-from-code-units.md
> 
> 2. The proposal doesn't show any usage of the proposed APIs, so it
>    is hard to understand what effect these APIs would have on real
>    code.  There are some examples of uses in
>    https://github.com/apple/swift/compare/master...zwaldowski:string-from-code-units
>    but I have to dig for them.
> 
> 3. The description of the “Detailed Design” doesn't show any examples
>    either, so I have to imagine what “backporting the Swift 3.0 versions
>    of the CString constructors, then making them generic over their
>    input and codec” means.

The notes are appreciated. I'll take care to add examples and fix
formatting. The proposal was always intended to be paired with the PR to
the stdlib, and that lack of clarity did not come up during the proposal
vetting stage.

> 
> Next, when I look at *uses* that I can find (in the tests), I don't find
> them to be clear.
> 
>   String(validatingCodeUnits: result, as: UTF8.self)
> 
> What does “validatingCodeUnits” mean?  Clearly we're going to do some
> checking.  Is there a repair?  Is the initializer failable?  

I'm open to suggestions. The names as they are were lifted from the
changes to the CString APIs made on swift-3-api-guidelines.

"repairingCodeUnits" implies to me that there is something already wrong
with them. I'm also not sure as to the point of the initializer being
failable; many domain initializers in the stdlib don't indicate that
they can fail directly in the name.

> 
> In this change, for example:
> https://github.com/apple/swift/compare/master...zwaldowski:string-from-code-units#diff-d39ec2c4819c950aeef95f899f53b104R79
> I find the old code much clearer than the new code (I'm not sure why
> _decode starts with an underscore here; isn't it part of your proposed
> public API?)

The PR is ahead of the proposal here. It was pointed out in the review
thread up to this point that that that "primitive" isn't truly needed;
the initializers on String should be the API used by all.

The ugliness in this instance is transitional; it stems from bridging
the old-style constructor to the new-style one. The old signature's
return type is awkward in that it returns the extra hadError flag
regardless of whether decoding failed, which is discarded almost
universally by clients in the stdlib.

I intend to revert the change to `fromCStringRepairingIllFormedUTF8`, or
elide `_decode` into it instead.

> 
> Lastly, I am very concerned about the “Alternatives Considered” section,
> where, of one alternative, it says:
> 
>       This might be the better long-term solution from the perspective
>       of API maintenance, but in the meantime this proposal has a fairly
>       low impact.
> 
> We can't accept changes into the standard library “in the meantime,”
> with the expectation that something more comprehensive will make them
> obsolete.  Even though we've had migration tools, we never operated that
> way in the past, and as we head toward API and ABI stability it is even
> more true today.

I understand the sentiment here, but I don't think "wait for the next
rewrite of String" is a good solution to a problem that the stdlib
already resolves. See below.

> 
> > Is the problem being addressed significant enough to warrant a change
> > to Swift?
> 
> Yes.
> 
> > Does this proposal fit well with the feel and direction of Swift?  
> 
> W.r.t. direction, the fact that we have a major String overhaul planned
> means that tackling this one corner of the API is probably not entirely
> appropriate.

"Corner" is an unfair characterization; users of Swift want to implement
things to the level of safety and performance bar set by the stdlib, but
we are currently at a disadvantage. No better example can be found but
in corelibs-foundation:
https://github.com/apple/swift-corelibs-foundation/blob/546dc8e16c3c34ca50f5752c6d0f39c3524f5f0a/Foundation/NSString.swift#L305.
Is it not a deficiency of the stdlib when code has to resort to
non-public methods? The stdlib (i.e., the parts touched by the PR) and
corelibs-foundation would have to move in lockstep to adopt a
replacement, so the existing underscored versions are as good as public
API.

I understand, and 100% encourage, the reticence around new API. However,
we've created a worse problem by encouraging slow, buggy, custom
versions of behavior that already exists in the stdlib, or implying that
the underscored API should be used because we haven't managed something
better yet. :/

> 
> > How much effort did you put into your review? A glance, a quick
> > reading, or an in-depth study?
> 
> between quick reading and in-depth study.
> 
> -- 
> -Dave
> 
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org
> https://lists.swift.org/mailman/listinfo/swift-evolution

More information about the swift-evolution mailing list