[do not merge] [stdlib] Improved pointers #11464

tayloraswift · 2017-08-15T00:31:37Z

CodaFi

Blocking this for visibility:

Throughout this diff there's a bunch of little whitespace changes. As a general policy, we like to have diffs as clean as possible. Could you go through and scrub the whitespace changes?

tayloraswift · 2017-08-19T18:40:20Z

oh that was Atom whenever it saves a file it strips the trailing whitespace i’ll see if i can fix that

xwu

Excited about these improvements! I've left some style notes below.

xwu · 2017-08-19T18:33:07Z

stdlib/public/core/Sequence.swift

+
+  @inline(__always)
+  public
+  func _copyContents( buffer: UnsafeMutableBufferPointer<Element>,


Here and elsewhere, can you make sure to follow prevailing stdlib convention, specifically:

Place access modifiers on the same line as func

Use no space after the opening parenthesis

Use no space before colon and one space after colon (unless stating conformances or inheritance, in which case use one space before and after)

Use two-space tabs and 80-character columns

Place braces on the same line as else, for, etc. (including on the same line as the function return type); place else on the same line as guard

Use no spaces before and after ..<

xwu · 2017-08-19T18:34:00Z

stdlib/public/core/StringCore.swift

-        (tailStart + (growth &<< elementShift)).copyBytes(
-          from: tailStart, count: tailCount &<< elementShift)
+        (tailStart + (growth &<< elementShift)).copy(
+        from: tailStart, bytes: tailCount &<< elementShift)


Here and below, please restore the original spacing, which indents two spaces when breaking after an opening parenthesis.

xwu · 2017-08-19T18:36:04Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  public
+  init(_ other:UnsafeMutableBufferPointer<Element>)
+  {
+    self._position = UnsafePointer<Element>(other._position)


Here and below, please follow prevailing style as discussed above; besides the style notes already mentioned (public on same line as init, space after :, { on same line as arguments), set internal members without prefixing self and do not align _end = with spaces.

xwu · 2017-08-19T18:36:45Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

 %  end # !mutable

+%  if Mutable:


Can't this be simply % else: after eliminating the % end above?

xwu · 2017-08-19T18:39:32Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  {
+    let size = MemoryLayout<Element>.stride * capacity
+    let raw  = Builtin.allocRaw(size._builtinWordValue, Builtin.alignof(Element.self))
+               Builtin.bindMemory(raw, capacity._builtinWordValue, Element.self)


Besides other style notes already mentioned above (including not using additional spaces in raw =), please indent only two spaces here.

xwu · 2017-08-19T18:40:05Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+    let size = MemoryLayout<Element>.stride * capacity
+    let raw  = Builtin.allocRaw(size._builtinWordValue, Builtin.alignof(Element.self))
+               Builtin.bindMemory(raw, capacity._builtinWordValue, Element.self)
+    return UnsafeMutableBufferPointer(start: UnsafeMutablePointer(raw),


Here, stdlib style is to line-break after the opening parenthesis and increase indent by two spaces on the next line.

xwu · 2017-08-19T18:40:30Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+                                      count: capacity)
+  }
+
+  // this is more efficient than the generic Sequence-based implementation


In comments, stdlib style is to capitalize and punctuate like a sentence.

xwu · 2017-08-19T18:41:47Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  public
+  func assign(from source:UnsafeBufferPointer<Element>)
+  {
+    _debugPrecondition(source.count >= self.count,


Ditto here about stdlib style and parentheses; I'll not repeat earlier comments about the styling of guard...else statements, which also apply here.

xwu · 2017-08-19T18:49:03Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  public
+  func initialize(repeating repeatedValue:Element)
+  {
+    guard let dst_start:UnsafeMutablePointer<Element> = self._position


Where type annotations are not necessary, stdlib style is to leave them off. Also, stdlib style is for camel-casing even local variables (i.e. dstStart).

atrick · 2017-08-19T20:56:51Z

I know, the whitespace thing is terribly annoying! I used to open/close a file to fix the whitespace then commit that separately before editing the file. Pure whitespace changes are also discouraged in Swift, but it's probably fine if it's limited to a file that you're already making large changes to.

atrick · 2017-08-19T21:26:59Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+
+  @_inlineable
+  public
+  func deinitialize()


Please add a scary warning comment to UnsafeMutableBufferPointer.deinitialize(). Something like:

Warning: All buffer elements must be initialized before calling this. Note that invoking initialize(from:) may not initialize all elements. Deinitializing part of the buffer must be done using the UnsafeMutablePointer API: baseAddress.deinitialize(count: n)

tayloraswift · 2017-08-19T23:54:04Z

I found out how to disable whitespace stripping in Atom but I have no idea how to restore the whitespace that was already there before Atom stripped it since it’s all mixed with the actual changes.

CodaFi · 2017-08-19T23:58:31Z

For the files with whitespace only changes git checkout master path/to/file will reset them. For the files with actual code changes you may have to do it by hand

tayloraswift · 2017-08-19T23:59:23Z

i only saved files i made changes to, which was about 20 or so because of function and argument renaming to make the tests compile

tayloraswift · 2017-08-20T00:35:09Z

does it look better?

xwu · 2017-08-20T00:59:05Z

stdlib/public/core/Sequence.swift

+  public func _copyContents( 
+    buffer: UnsafeMutableBufferPointer<Element>,
+    _ operation: (UnsafeMutablePointer<Element>, Element) -> ())
+    -> (Iterator, UnsafeMutableBufferPointer<Element>.Index) {


One more stdlib-specific style note: the prevailing convention is to put the closing parenthesis, deindented, next to the return arrow: ) ->.

xwu · 2017-08-20T01:00:39Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

-    where S.Element == Element {
+  @discardableResult
+  public func initialize<S>(from source: S) -> (S.Iterator, Index)
+    where S:Sequence, S.Element == Element {


Style nit: missing spaces here (and below); should be S : Sequence.

xwu · 2017-08-20T01:01:45Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+    _debugPrecondition(source.count >= count,
+                      "${Self}.moveAssign(from:) source does not have enough elements")
+    guard let dstStart = _position,
+          let srcStart = source._position else {


Nit: I think this now all fits on one line :)

is there a stdlib convention for this? my own convention is to always put guard and if comma statements on their own lines

In the stdlib, if the whole thing fits in 80 characters, you'll see everything in one line:

guard let foo = _foo, let bar = _bar else { return }

(Which is not to say, of course, that your style is inferior in any way.)

xwu · 2017-08-20T01:07:06Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  @_inlineable
+  public
+  func withMemoryRebound<T, Result>(
+    to type: T.Type, _ body: (${Self}<T>) throws -> Result) rethrows -> Result {


Style nit: This one's tricky, but public func should be one line, then probably to balance out the opening (, you could line break and deindent before )--although I'm not sure about this one; check elsewhere in the codebase.

Below, where you've got body(${Self}<T>( and count: on a separate line, line return after the opening parenthesis and indent two spaces, then put everything else on that line.

xwu · 2017-08-20T01:12:50Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  /// a buffer can have a `count` of zero even with a non-`nil` base address.
+  @_inlineable
+  public var count: Int {
+    switch(_position, _end) {


Style nits: space after switch; default instead of case _ (which isn't used in stdlib unless i'm mistaken).

More generally, though, isn't this more idiomatically just:

guard let position = _position, let end = _end else { return 0 } return end - position

i don’t think i ever touched this part of the code, the diff is probably because a method near it got moved

Fair; since the diff now points to you, it could still stand a little stylistic tidying though :)

xwu · 2017-08-20T01:13:34Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  }
+
+  @_versioned
+  let _position, _end: Unsafe${Mutable}Pointer<Element>?


Style nit: I believe stdlib always explicitly marks internal members as internal.

again, wasn’t me

xwu

Some more style nits (sorry!).

xwu · 2017-08-20T01:22:53Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  /// A textual representation of the buffer, suitable for debugging.
+  public var debugDescription: String {
+    return "Unsafe${Mutable}BufferPointer"
+      + "(start: \(_position.map(String.init(describing:)) ?? "nil"), count: \(count))"


A good place for a multiline string literal?

again, wasn’t me
(also i never did learn how to use those things after the argument over trailing newlines after \ went south lol)

xwu · 2017-08-20T01:25:23Z

stdlib/public/core/UnsafeRawPointer.swift.gyb

-  public static func allocate(
-    bytes size: Int, alignedTo: Int
-  ) -> UnsafeMutableRawPointer {
+  public static func allocate(bytes: Int, alignedTo alignment: Int) 


Nit: the original line breaking here was consistent with prevailing stdlib convention.

xwu · 2017-08-20T01:25:47Z

stdlib/public/core/UnsafeRawPointer.swift.gyb

-  public func bindMemory<T>(
-    to type: T.Type, capacity count: Int
-  ) -> Unsafe${Mutable}Pointer<T> {
+  public func bindMemory<T>(to type: T.Type, capacity count: Int = 1)


Nit: ditto here.

xwu · 2017-08-20T01:26:51Z

stdlib/public/core/UnsafeRawPointer.swift.gyb

@@ -835,6 +838,10 @@ public struct Unsafe${Mutable}RawPointer : Strideable, Hashable, _Pointer {
    }
  }

+  @available(*, unavailable, renamed: "copy(from:bytes:)")
+  public func copyBytes(from source: UnsafeRawPointer, count: Int)
+  { fatalError("unreachable") }


Nit: move { to previous line and } into a separate line.

i thought mini blocks weren’t to be formatted like “real” function bodies, like a.map{ $0.foo } instead of

a.map { $0.foo }

All function bodies have the following format (unless it all fits in 80 characters):

public func foo() { fatalError() }

See: https://github.com/apple/swift/blob/f6f3ed0fe7e3c0d8d6af85bb238e039041ac0d42/validation-test/stdlib/CollectionDiagnostics.swift#L15

xwu · 2017-08-20T01:28:05Z

stdlib/public/core/UnsafeRawPointer.swift.gyb

-  ) -> UnsafeMutablePointer<T> {
+  public func initializeMemory<T>( 
+    as type: T.Type, at index:Int = 0, repeating repeatedValue:T, count: Int = 1)
+    -> UnsafeMutablePointer<T> {


Nit: as before, ) -> is the prevailing style.

xwu · 2017-08-20T01:28:17Z

stdlib/public/core/UnsafeRawPointer.swift.gyb

-    as type: T.Type, from source: UnsafePointer<T>, count: Int
-  ) -> UnsafeMutablePointer<T> {
+    as type: T.Type, from source: UnsafePointer<T>, count: Int = 1)
+    -> UnsafeMutablePointer<T> {


Nit: ditto here.

xwu · 2017-08-20T01:28:36Z

stdlib/public/core/UnsafeRawPointer.swift.gyb

-    as type: T.Type, from source: UnsafeMutablePointer<T>, count: Int
-  ) -> UnsafeMutablePointer<T> {
+    as type: T.Type, from source: UnsafeMutablePointer<T>, count: Int = 1)
+    -> UnsafeMutablePointer<T> {


...and here.

natecook1000

Looking great! I've added a few notes about where we need to make sure we're keeping documentation in line with the changes.

natecook1000 · 2017-08-21T16:11:36Z

stdlib/public/core/UnsafePointer.swift.gyb

@@ -429,6 +429,11 @@ public struct ${Self}<Pointee>
    return UnsafeMutablePointer(rawPtr)
  }

+  @available(*, unavailable, message: "Swift currently only supports freeing entire heap blocks, use deallocate() instead")


Using unavailable in these changes will break existing code. Should this be something like @available(swift, deprecated: 4.1, obsoleted: 5, message: "...")? Do we have the machinery in place for those versions yet?

as explained in the proposal we are preferring a hard unavailable over a soft deprecation.

I see that, but using unavailable causes source breakage of Swift 4 code, not just code that is migrated to Swift 5.

natecook1000 · 2017-08-21T16:14:30Z

stdlib/public/core/UnsafePointer.swift.gyb

@@ -488,14 +494,14 @@ public struct ${Self}<Pointee>
  ///   - count: The number of consecutive copies of `newValue` to initialize.
  ///     `count` must not be negative. The default is `1`.
  @_inlineable
-  public func initialize(to newValue: Pointee, count: Int = 1) {
+  public func initialize(repeating repeatedValue: Pointee, count: Int = 1) {


Please update docs when the API changes—in this case, the name of the method is now intialize(repeating:count) and the first parameter name has changed from newValue to repeatedValue.

Thanks for taking care of these!

natecook1000 · 2017-08-21T16:16:35Z

stdlib/public/core/UnsafePointer.swift.gyb

-  public func assign(from source: UnsafePointer<Pointee>, count: Int) {
-    _debugPrecondition(
-      count >= 0, "${Self}.assign with negative count")
+  public func assign(from source: UnsafePointer<Pointee>, count: Int = 1) {


Default parameter values need to be specified in the docs for the parameter: "The default value is 1". Noted here and 4–5 other places below.

natecook1000 · 2017-08-21T16:23:11Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

-%  end # !mutable
+
+  @_inlineable
+  public init(_ other: UnsafeMutableBufferPointer<Element>) {


Please make sure any new public APIs have documentation before this gets merged—thanks!

gottesmm · 2017-08-21T21:23:17Z

@Kelvin13 Please make sure to rebase the branch on master before merging!

tayloraswift · 2017-08-22T01:22:52Z

The implementation is now up to speed with the latest version of the draft proposal

harlanhaskins requested review from atrick and airspeedswift and removed request for atrick August 16, 2017 17:12

CodaFi requested changes Aug 19, 2017

View reviewed changes

xwu reviewed Aug 19, 2017

View reviewed changes

atrick reviewed Aug 19, 2017

View reviewed changes

xwu reviewed Aug 20, 2017

View reviewed changes

CodaFi approved these changes Aug 20, 2017

View reviewed changes

natecook1000 requested changes Aug 21, 2017

View reviewed changes

jessesquires mentioned this pull request Aug 21, 2017

[84] Issue #84 - August 24, 2017 SwiftWeekly/swiftweekly.github.io#298

Closed

natecook1000 approved these changes Aug 26, 2017

View reviewed changes

natecook1000 changed the title ~~Improved pointers (do not merge)~~ [do not merge] [stdlib] Improved pointers Aug 26, 2017

tayloraswift closed this Sep 30, 2017

[do not merge] [stdlib] Improved pointers #11464

[do not merge] [stdlib] Improved pointers #11464

Conversation

tayloraswift commented Aug 15, 2017

CodaFi left a comment

Choose a reason for hiding this comment

tayloraswift commented Aug 19, 2017

xwu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xwu Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xwu Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xwu Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atrick commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tayloraswift commented Aug 19, 2017

CodaFi commented Aug 19, 2017

tayloraswift commented Aug 19, 2017

tayloraswift commented Aug 20, 2017

Choose a reason for hiding this comment

xwu Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xwu Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tayloraswift Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

xwu Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xwu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tayloraswift Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tayloraswift Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

xwu Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natecook1000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tayloraswift Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gottesmm commented Aug 21, 2017

tayloraswift commented Aug 22, 2017

xwu Aug 19, 2017 •

edited

Loading

xwu Aug 19, 2017 •

edited

Loading

xwu Aug 19, 2017 •

edited

Loading

xwu Aug 20, 2017 •

edited

Loading

xwu Aug 20, 2017 •

edited

Loading

tayloraswift Aug 20, 2017 •

edited

Loading

xwu Aug 20, 2017 •

edited

Loading

xwu left a comment •

edited

Loading

tayloraswift Aug 20, 2017 •

edited

Loading

tayloraswift Aug 20, 2017 •

edited

Loading

xwu Aug 20, 2017 •

edited

Loading

tayloraswift Aug 22, 2017 •

edited

Loading