- Notifications
You must be signed in to change notification settings - Fork 10.6k
[stdlib] Documentation revisions #11249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| @swift-ci Please smoke test |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
f367e86 to a00c713 Compare - 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
a00c713 to 8329ca9 Compare | @swift-ci Please smoke test |
1 similar comment
| @swift-ci Please smoke test |
milseman left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, agreed!
There was a problem hiding this comment.
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…
atrick left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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:)?
There was a problem hiding this comment.
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!
[stdlib] Documentation revisions
These revisions touch on a variety of areas, mostly focusing on recent changes to the stdlib.