Eager Bridging 2: Eager Bridgealoo (do not merge) #5077
Conversation
This failure mode can no longer be reliably detected.
Two tests need to be fixed up, but first I want to reconsider how _ObjectiveCBridgeable works.
|
Ah, had some residue from the rebase. Clean build fixed things up and I found the real problem. Builds and passes tests locally. |
|
@swift-ci please test |
| @@ -238,7 +231,7 @@ extension String { | |||
|
|
|||
| // FIXME: can't just use a default arg for radix below; instead we | |||
| // need these single-arg overloads <rdar://problem/17775455> | |||
|
|
|||
There was a problem hiding this comment.
We've decided as a project to to avoid introducing changes that strip trailing whitespace. They add noise to the commit history and to code review, with no benefit.
There was a problem hiding this comment.
Sure thing, editor preferences tweaked. Will snip these out.
There was a problem hiding this comment.
btw, if you add ?w=0 to any url that provides a diff it will supress any whitespace changes. It will even suppress changes that are "only" re-indenting (which is actually pretty relevant to this patch, which is making a lot of if'd code unconditional).
e.g.
(check out StringComparable.swift for a good example)
There was a problem hiding this comment.
Whitespace changes annihilated.
Gankra
force-pushed
the
eager-bridging2
branch
from
68748df to
aff254c
Compare
|
@dabrahams I recall you saying you've seen the compiler's problematic behaviour with extension conformances to KnownProtocols before. Are you aware of a radar for it, or should I file a new one? |
| var nsRoundTripUTF16 = newNSUTF16 as NSString | ||
| print(" \(repr(nsRoundTripUTF16))") | ||
|
|
||
| // CHECK: --- UTF-16 slicing --- | ||
| print("--- UTF-16 slicing ---") | ||
|
|
||
| // Slicing the String does not allocate | ||
| // CHECK-NEXT: String(Contiguous(owner: .cocoa@[[utf16address]], count: 6)) | ||
| // CHECK-NEXT: String(Contiguous(owner: .native@[[utf16address]][-6...0], capacity = 16)) |
There was a problem hiding this comment.
Don't do equality checks on capacity, allocator behavior is different across platforms.
There was a problem hiding this comment.
Yeah I wasn't sure on this. I considered ignoring it, or asserting it didn't change, but I looked around and saw some files asserting exact comparisons in similar tests.
Want me to change those too?
| } else { | ||
| start = UnsafeMutableRawPointer(mutating: nulTerminatedASCII) | ||
| } | ||
|
|
||
| self._core = _StringCore( |
There was a problem hiding this comment.
Please be careful not to remove the ASCII string optimization.
There was a problem hiding this comment.
Are the precise details of this doc'd anywhere so I can verify that I'm upholding it?
There was a problem hiding this comment.
'fraid not. If the string is ASCII, we try to store it as such, and avoid branching by using math to adjust our accesses to use 1 or 2 bytes at a time.
| // Look first for null-terminated ASCII | ||
| // Note: the code in clownfish appears to guarantee | ||
| // nul-termination, but I'm waiting for an answer from Chris Kane | ||
| // about whether we can count on it for all time or not. |
There was a problem hiding this comment.
I know I said you could delete this comment, but now that I read it I see that it's directly relevant to what follows.
There was a problem hiding this comment.
Can you elaborate? Like it obviously relates to this code, I'm just not clear why it's an interesting thing. Wild guess: this is a terrible proxy for "is ASCII" if it constantly fails out on missing nul-terminators?
|
Having finally gotten around to checking it out, it looks like #3046 massively conflicts with this work for Dictionary/Set. I think those eager bridges should be split out to be implemented in coordination with that effort (possibly, it should just be blocked on it, or maybe just merged with it?). I'd feel bad about just completely trashing the work in that PR. Anyway, it looks like String/Array are the really interesting cases for evaluating the impact of this change. |
|
Closing in favour of the three PRs that this was split into. |
|
on Fri Nov 11 2016, Alexis Beingessner <notifications-AT-i.8713187.xyz> wrote:
@gankro it would be nice to list the other PRs in the one you're closing. |
This is a continuation of the work in #4514
I've unfortunately been having some trouble with illness so this is pretty sloppy. I'm posting this up now to discuss the details while I rest. Something that needs more evaluation is the impact this has on the performance of "pure Swift code". A few methods on String (lower casing and upper casing being notable) will do a roundtrip through NSString.
My latest rebase to master doesn't seem to build, but it looks unrelated to these actual changes (master is broken?).