Skip to content

Conversation

@natecook1000
Copy link
Member

These revisions touch on a variety of areas, mostly focusing on recent changes to the stdlib.

@natecook1000
Copy link
Member Author

@swift-ci Please smoke test

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this isn't quite right. Per the IEEE specification:

NOTE—totalOrder does not impose a total ordering on all encodings in a format. In particular, it does not distinguish among different encodings of the same floating-point representation, as when one or both encodings are non-canonical.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you feel about actually documenting how the total ordering works here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. So I know that our implementation does distinguish between encodings, so what we're saying here that isTotallyOrdered imposes the IEEE total order relationship and further imposes a total order even on noncanonical encodings.

Is the actual order mandated by the spec? If it's not I don't know that we want to guarantee a specific ordering of the various elements. It's not immediately apparent from the implementation what ends up where.

@stephentyrone?

Copy link
Collaborator

@xwu xwu Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual order is mandated by the specification in gory detail. The default implementation for BinaryFloatingPoint implements exactly the spec. The result is that some encodings of the same noncanonical encodings may be distinguished from each other, but not all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, got it! Updated that and then noticed that the example (a) had the wrong parameter label and (b) didn't have the correct semantics... 🙄 Going to come back to the Q about specifying the order later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually wish that method was static func areInIncreasingTotalOrder(_ x: Self, _ y: Self) -> Bool—would play more nicely with sorting.

Copy link
Collaborator

@xwu xwu Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see your point, but IEEE totalOrder actually can't be used as-is as a Swift sorting predicate: it is required to return true if two values are equal, whereas areInIncreasingOrder should return false.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think justification is required; in general, we should need to justify why certain facilities are operators and not why others are not.

By including this statement, I worry that you're prompting people to wonder whether they actually want to use this operation more often than they thought, as the implication is that an operator spelling was a real possibility that was considered and rejected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I think this language might have originated in the floating-point protocols proposal, where it made more sense. I'll look at removing it.

- Revisions to unsafeDowncast and withVaList - Fix the Int64/UInt64 discussion - Buffer pointer revisions - Fix Optional example to use new integer methods - Revise and correct some UnsafeRawBufferPointer docs - Fix symmetricDifference examples - Fix wording in FloatingPoint.nextDown - Update ImplicitlyUnwrappedOptional - Clarify elementsEqual - Minor integer doc fixes - Comment for _AppendKeyPath - Clarification re collection indices - Revise RangeExpression.relative(to:) - Codable revisions
- Clarify StringProtocol conformance - Deprecate ExpressibleByStringInterpolation - String index conversions docs - Describe shared string indices
@natecook1000
Copy link
Member Author

@swift-ci Please smoke test

1 similar comment
@moiseev
Copy link
Contributor

moiseev commented Jul 31, 2017

@swift-ci Please smoke test

@natecook1000 natecook1000 merged commit 5d17b31 into swiftlang:master Jul 31, 2017
Copy link
Member

@milseman milseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, awesome!

/// If the index passed as `sourcePosition` represents the start of an
/// extended grapheme cluster---the element type of a string---then the
/// initializer succeeds. If the index instead represents the position of a
/// Unicode scalar within an extended grapheme cluster or the position of an
Copy link
Member

@milseman milseman Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this wording needs to have anything to do with Unicode scalars. The semantics are specifically that if the index lies on a grapheme boundary, then it is valid. Non-grapheme boundary indices (whether on scalar boundaries or sub-scalar indices) return nil.

edit: typos

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, agreed!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that's weird is that the position of a Unicode scalar that is a valid start of a grapheme cluster will convert, even if it's inside what the string is treating as an extended cluster. For example:

let astro = "👩‍🚀" // astro.count == 1 let i = astro.unicodeScalars.index(astro.unicodeScalars.startIndex, offsetBy: 2) // astro[i] == "🚀" let j = String.Index(i, within: astro)! // astro[j] == "🚀" // astro[astro.startIndex] == "👩‍🚀"

That seems a little odd to me, though it still agrees with the letter of the documentation. We'd have to introduce some kind of backtracking scheme (or maybe even start from the beginning of the string again?) to have it not do this. That could easily end up killing the performance for this kind of check…

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

/// Initializes the buffer's memory with the given elements, binding the
/// initialized memory to the elements' type.
///
/// When calling the `initialize(as:from:)` method on a buffer `b`, the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean initializeMemory(as:from:)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, thanks for catching that!

natecook1000 added a commit to natecook1000/swift that referenced this pull request Aug 3, 2017
@natecook1000 natecook1000 deleted the nc-rev-77-1 branch October 4, 2018 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants