Skip to content

SE 0184a implementation #12200

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

Merged
merged 1 commit into from
Nov 18, 2017
Merged

SE 0184a implementation #12200

merged 1 commit into from
Nov 18, 2017

Conversation

tayloraswift
Copy link
Member

@tayloraswift tayloraswift commented Sep 30, 2017

Copy link
Collaborator

@xwu xwu left a comment

Choose a reason for hiding this comment

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

This is awesome; so glad that you're pushing ahead with this change! Just two minor comments on the documentation, and some formatting nits.

(tailStart + (growth &<< elementShift)).copyBytes(
from: tailStart, count: tailCount &<< elementShift)
(tailStart + (growth &<< elementShift)).copyMemory(
from: tailStart, byteCount: tailCount &<< elementShift)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could you please restore the indentation here?

/// Deallocates the heap block previously allocated at this buffer pointer’s
/// base address.
///
/// This buffer pointer’s `baseAddress` must be `nil` or a pointer to a previously
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are nicely documented!

Small nit here and elsewhere below: could you please straighten the apostrophe? I believe it interferes with some downstream tooling and have seen others go through and do this sort of clean-up; might as well have it straightened in the first place.

/// pointer.
///
/// When you allocate memory, always remember to deallocate once you're
/// finished.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's certainly important, but could we choose only one of the two sentences above? They say exactly the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

this duplication also already exists in UnsafeMutablePointer

/// to be the same size as its `Pointee` type.
@_inlineable
public func withMemoryRebound<T, Result>(
to type: T.Type, _ body: (${Self}<T>) throws -> Result) rethrows -> Result {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Formatting nit: typically, in stdlib style, when the parameters start on another line, the closing parenthesis is also on a separate line like so:

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

Builtin.bindMemory(base._rawValue, count._builtinWordValue, Element.self)
}

return try body(${Self}<T>( start: Unsafe${Mutable}Pointer<T>(base._rawValue),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: extra space after opening parenthesis, although stdlib formatting would actually call for the following:

return try body(${Self}<T>(
  start: Unsafe${Mutable}Pointer<T>(base._rawValue), count: capacity))

public func initializeMemory<T>(as type: T.Type, at index: Int = 0,
count: Int = 1, to value: T
public func initializeMemory<T>(
as type: T.Type, repeating repeatedValue:T, count: Int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: space after repeatedValue:; remove space at the end of the previous line.

(rawPtr + MemoryLayout<Int>.stride).copyBytes(
from: roPtr, count: 2 * MemoryLayout<Int>.stride)
(rawPtr + MemoryLayout<Int>.stride).copyMemory(
from: roPtr, byteCount: 2 * MemoryLayout<Int>.stride)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: please restore indent.

rawPtr.copyBytes(
from: roPtr + MemoryLayout<Int>.stride, count: 2 * MemoryLayout<Int>.stride)
rawPtr.copyMemory(
from: roPtr + MemoryLayout<Int>.stride, byteCount: 2 * MemoryLayout<Int>.stride)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: please restore indent.

rawPtr.copyBytes(
from: roPtr + 2 * MemoryLayout<Int>.stride, count: 2 * MemoryLayout<Int>.stride)
rawPtr.copyMemory(
from: roPtr + 2 * MemoryLayout<Int>.stride, byteCount: 2 * MemoryLayout<Int>.stride)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: please restore indent.

(rawPtr + 2 * MemoryLayout<Int>.stride).copyBytes(
from: roPtr, count: 2 * MemoryLayout<Int>.stride)
(rawPtr + 2 * MemoryLayout<Int>.stride).copyMemory(
from: roPtr, byteCount: 2 * MemoryLayout<Int>.stride)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: please restore indent.

@tayloraswift
Copy link
Member Author

@xwu better?

@gottesmm
Copy link
Contributor

gottesmm commented Oct 1, 2017

@Kelvin13 Please rebase instead of merging in on feature branches. I am going to kick off a run and put a [WIP] don't merge in the title.

@gottesmm gottesmm changed the title SE 0184a implementation [WIP] SE 0184a implementation. DON'T MERGE Oct 1, 2017
@gottesmm
Copy link
Contributor

gottesmm commented Oct 1, 2017

@swift-ci test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 1, 2017

Build failed
Swift Test Linux Platform
Git Sha - 6c14eec80f3d4db0462c7c915cd06e6231a3d9f8

@swift-ci
Copy link
Contributor

swift-ci commented Oct 1, 2017

Build failed
Swift Test OS X Platform
Git Sha - 6c14eec80f3d4db0462c7c915cd06e6231a3d9f8

@tayloraswift
Copy link
Member Author

@swift-ci test

@moiseev
Copy link
Contributor

moiseev commented Oct 2, 2017

Since we're removing sil-serialize-all mode for the standard library at the moment, please mark all the new @_inlineable attributes that you're not sure about with a FIXME(sil-serialize-all): to be reviewed later.

@moiseev
Copy link
Contributor

moiseev commented Oct 2, 2017

/cc @natecook1000 for the documentation changes.

@moiseev
Copy link
Contributor

moiseev commented Oct 2, 2017 via email

@jckarter
Copy link
Contributor

jckarter commented Oct 2, 2017

@swift-ci Please test

@swiftlang swiftlang deleted a comment from swift-ci Oct 2, 2017
@swiftlang swiftlang deleted a comment from swift-ci Oct 2, 2017
@jckarter
Copy link
Contributor

jckarter commented Oct 2, 2017

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2017

Build failed
Swift Test Linux Platform
Git Sha - 92e80cf35f2ad6312075cafe7d9321d711adb653

@natecook1000 natecook1000 self-requested a review October 2, 2017 22:41
@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2017

Build failed
Swift Test OS X Platform
Git Sha - 92e80cf35f2ad6312075cafe7d9321d711adb653

@jckarter
Copy link
Contributor

jckarter commented Oct 2, 2017

Please test with the following PR:

swiftlang/swift-corelibs-foundation#1247

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2017

Build failed
Swift Test Linux Platform
Git Sha - 92e80cf35f2ad6312075cafe7d9321d711adb653

@jckarter
Copy link
Contributor

jckarter commented Oct 3, 2017

Please test with the following PR:

swiftlang/swift-corelibs-foundation#1247

@swift-ci Please clean test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 3, 2017

Build failed
Swift Test Linux Platform
Git Sha - 92e80cf35f2ad6312075cafe7d9321d711adb653

@swift-ci
Copy link
Contributor

swift-ci commented Oct 3, 2017

Build failed
Swift Test OS X Platform
Git Sha - 92e80cf35f2ad6312075cafe7d9321d711adb653

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

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

Looking great! Just a few notes inline.

///
/// Warning: Unlike this method, the destination type `T` of
/// `Unsafe${Mutable}Pointer.withMemoryRebound(to:capacity:_:)` requires `T`
/// to be the same size as its `Pointee` type.
Copy link
Member

Choose a reason for hiding this comment

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

This warning doesn't need to be 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.

yes it does. it has to do with how the type gets rebound by the builtins.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we don't need a warning about the usage of an UnsafePointer method here in the documentation for an UnsafeBufferPointer method. The documentation for UnsafePointer.withMemoryRebound already covers this precondition, and it isn't relevant here. (If this is meant to be a code comment, then it's fine, but it should double-slashed and in the method body.)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe an even better option would be to relax the precondition in UnsafePointer.withMemoryRebound so that the two similar methods have the same semantics. cc @atrick

Copy link
Contributor

Choose a reason for hiding this comment

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

The buffer version of withMemoryRebound has an additional constraint. MemoryLayout<T>.stride == MemoryLayout<Element>.stride. This is necessary because the user doesn't provide a count for the resulting number of T elements. This buffer API simply doesn't make sense without this precondition.

I think the warning reads wrong. It sounds the precondition is on the UnsafePointer API, but that's wrong.

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 don’t really care about this use case enough to hold up the proposal over it, but what if you want to fuse a buffer of UInt8s into UInt32s or something?

let integers:[UInt32] = bytes.withMemoryRebound(to: UInt32.self) 
{
    return $0.map(UInt32.init(bigEndian:))
}

My PNG library does this a lot but with raw buffer pointers as a workaround. js. if you still disagree i’ll just take it out. It’ll save us an integer division at least

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a great use case for raw buffers!

Well, your implementation is memory safe for trivial values... However, even if we came up with a set of rules that allowed withMemoryRebound to reinterpret memory as differently sized integers (which is probably not a good idea), then in practice the integer division alone is a serious sticking point 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.

You’re probably right tbh. Using raw buffers is a bit more awkward for things like matrix buffers where you have a bunch of matrices stored back to back as an array of Floats. But that’s really a job for fixed sized arrays, not buffer pointer rebinds.

Copy link
Member Author

Choose a reason for hiding this comment

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

someone run the ci pls

Copy link
Member Author

Choose a reason for hiding this comment

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

@atrick you have to do it twice because of the sha

@@ -366,6 +389,140 @@ public struct Unsafe${Mutable}BufferPointer<Element>
public func makeIterator() -> UnsafeBufferPointerIterator<Element> {
return UnsafeBufferPointerIterator(_position: _position, _end: _end)
}

/// Deallocates the heap block previously allocated at this buffer pointer’s
/// base address.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we're using "heap block" anywhere else in these docs. Can you use "memory region" or "memory block" instead?

/// allocated memory block. If `baseAddress` is `nil`, this function does nothing.
/// Otherwise, the memory must not be initialized or `Pointee` must be a trivial
/// type. This buffer pointer's `count` must be equal to the allocated
/// size of the heap block.
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it clear that you only use deallocate() with the same pointer you received from allocation? That's one of the main clarifying points of this change—would be great to make sure that concept is in the documentation (here and in the other deallocate() methods).

/// type `Element`.
///
/// The resulting buffer references a region of memory that is bound to
/// `Element` and is `count * MemoryLayout<Element>.stride` bytes in size.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add an example of allocation similar to what's in the UnsafePointer docs here:

/// The following example allocates a buffer that can store four `Int` 
/// instances and then initializes that memory with the elements of a range:
/// 
///     let buffer = UnsafeMutableBufferPointer<Int>.allocate(capacity: 4)
///     _ = buffer.initialize(from: 1...4)
///     print(buffer[2])
///     // Prints "3"

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@jckarter
Copy link
Contributor

jckarter commented Oct 3, 2017

Please test with the following PR:

swiftlang/swift-corelibs-foundation#1247

@swift-ci Please clean test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 3, 2017

Build failed
Swift Test Linux Platform
Git Sha - 92e80cf35f2ad6312075cafe7d9321d711adb653

@swift-ci
Copy link
Contributor

swift-ci commented Oct 3, 2017

Build failed
Swift Test OS X Platform
Git Sha - 92e80cf35f2ad6312075cafe7d9321d711adb653

@jckarter
Copy link
Contributor

jckarter commented Oct 3, 2017

Please test with the following PR:

swiftlang/swift-corelibs-foundation#1247

@swift-ci Please clean test

Copy link
Member

@airspeedswift airspeedswift left a comment

Choose a reason for hiding this comment

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

This PR needs a handful of changes to address source compatibility.

It's good to update all the tests to use the new form (which can be immediately available so long as they don't cause ambiguity with the compatibility versions), but there should also be some tests confirming the compatibility ones work too.

@@ -852,29 +861,34 @@ public struct Unsafe${Mutable}RawPointer : Strideable, Hashable, _Pointer {
}
}

@available(*, unavailable, renamed: "copyMemory(from:byteCount:)")
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be gated by a version number. It should continue to work until Swift 5.

/// - Returns: A raw pointer to the same address as this pointer. The memory
/// referenced by the returned raw pointer is still bound to `Pointee`.
@_inlineable
@discardableResult
public func deinitialize(count: Int = 1) -> UnsafeMutableRawPointer {
public func deinitialize(count: Int) -> UnsafeMutableRawPointer {
Copy link
Member

Choose a reason for hiding this comment

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

This needs a compatibility shim to ensure you can still call it without an argument until 5.0

Copy link
Member Author

Choose a reason for hiding this comment

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

This can’t be overloaded because deinitialize(count:) with the default arg resolves to the same symbol as deinitialize(count:) without the default arg.

Copy link
Member

Choose a reason for hiding this comment

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

You can add a no-argument method that forwards to the new version, and make it obsoleted in 5.

Copy link
Member Author

Choose a reason for hiding this comment

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

isn’t that a completely new symbol?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but that’s fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -488,28 +486,48 @@ public struct ${Self}<Pointee>
}

% if mutable:
@available(*, unavailable, renamed: "initialize(repeating:count:)")
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be obsoleted: 5.0 instead of just unavailable.

@@ -488,28 +486,48 @@ public struct ${Self}<Pointee>
}

% if mutable:
@available(swift, obsoleted: 5.0.0, renamed: "initialize(repeating:count:)")
public func initialize(to repeatedValue: Pointee, count:Int = 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: space after count:.

Also, the function that's being obsoleted is technically initialize(to newValue: Pointee, count: Int = 1) (as opposed to repeatedValue). Doesn't strictly matter for compatibility, but the name is more correct in the circumstance that count defaults to 1.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@_inlineable
public func initialize(to newValue: Pointee, count: Int = 1) {
public func initialize(repeating repeatedValue: Pointee, count: Int) {
// FIXME: add tests (since the `count` has been added)
_debugPrecondition(count >= 0,
"${Self}.initialize(to:): negative count")
Copy link
Contributor

@glessard glessard Oct 11, 2017

Choose a reason for hiding this comment

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

This _debugPrecondition message references the old argument label

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

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.

Looks great. I just want to understand the choice of availability annotations better before accepting it.

@@ -329,7 +329,30 @@ public struct Unsafe${Mutable}BufferPointer<Element>
self.init(start: slice.base.baseAddress! + slice.startIndex,
count: slice.count)
}
% end # !mutable
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: I find it easier to read and maintain .gyb files when only closely related overloads are grouped under the same % if block. Your initializers are unrelated to the rebasing initializer.

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 tried to avoid touching the gyb lines as much as possible mostly because i don’t know gyb

/// value is also used as the return value for the `withMemoryRebound(to:_:)`
/// method.
/// - Returns: The return value, if any, of the `body` closure parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an extra newline here.

@available(*, deprecated, message: "Swift currently only supports freeing entire heap blocks, use deallocate() instead")
public func deallocate(capacity _: Int) {
self.deallocate()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@airspeedswift Do we also need to mark this obsolete in Swift 5 so we don't need to carry forward the symbol? (I'm not sure how these availability attributes are supposed to be used).

@available(*, unavailable, renamed: "allocate(byteCount:alignment:)")
public static func allocate(count: Int) -> UnsafeMutableRawBufferPointer {
fatalError("unreachable")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@airspeedswift I interpret unavailable as instantly obsoleted in all modes. (I can't find docs on these attributes.) Why don't we need source-compatibility in cases like this? Does it make a difference that we can use renamed?

bytes size: Int, alignedTo: Int
) -> UnsafeMutableRawPointer {
fatalError("unreachable")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question: Why not obsoleted: 5?

@available(*, deprecated, message: "Swift currently only supports freeing entire heap blocks, use deallocate() instead")
public func deallocate(bytes _: Int, alignedTo _: Int) {
self.deallocate()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to deprecating, do we also need to obsolete this in 5?

Copy link
Member Author

@tayloraswift tayloraswift Oct 17, 2017

Choose a reason for hiding this comment

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

i don’t know,,, the way i’ve always used @available is to deprecate stuff that’s being removed and unavailable–rename stuff that’s been renamed. that’s just the convention i follow in my projects, the swift book is not very helpful here

Copy link
Member

Choose a reason for hiding this comment

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

You can supply multiple @available attributes - that attribute is the first one documented here.

Copy link
Member Author

@tayloraswift tayloraswift Oct 17, 2017

Choose a reason for hiding this comment

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

okay but it doesn’t say what the exact difference between unavailable and obsoleted and renamed is or when to use each one. i’ve just been going off of the example they have midway down the page

@available(*, unavailable, renamed: "MyRenamedProtocol")

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so the swift-book says that you can use renamed with unavailable, but it actually works with the other availability attributes as well. I think we typically want to use obsoleted: 5, renamed: so that we have source compatibility in Swift 4 mode but aren't forced to provide those symbols in Swift 5. I'll need to check with @airspeedswift to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

does that mean the legacy function implementations have to stay instead of being replaced with fatalError("unreachable")??

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 changed all the unavailables to obsoleteds and i did my best to forward them to the actual implementations

public func initializeMemory<T>(
as type: T.Type, at offset: Int = 0, count: Int = 1, to repeatedValue: T
) -> UnsafeMutablePointer<T> {
_debugPrecondition(offset == 0,
Copy link
Member

Choose a reason for hiding this comment

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

This new precondition will break the behavior of existing code that uses a nonzero at parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

the behavior would already be broken anyway since the new implementation doesn’t take an offset parameter, so it wouldn’t get forwarded from the compatibility shim. The precondition just warns users about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

nvm i just used (self + offset * MemoryLayout<T>.stride) in the shim body

Copy link
Member

Choose a reason for hiding this comment

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

Great solution! 👍

@@ -439,22 +437,22 @@ public struct ${Self}<Pointee>
Builtin.bindMemory(rawPtr, count._builtinWordValue, Pointee.self)
return UnsafeMutablePointer(rawPtr)
}

@available(swift, obsoleted: 5.0.0, message: "Swift currently only supports freeing entire heap blocks, use deallocate() instead")
Copy link
Member

Choose a reason for hiding this comment

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

We should deprecate all these methods that are going away, so users get information about the change even before they've migrated to Swift 5. You can add:

@available(swift, deprecated, renamed/message: "same as used for the obsoleted attribute") 

Copy link
Member

Choose a reason for hiding this comment

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

Well, the compiler doesn't actually like that, but you can do even better. Add a deprecated parameter alongside the obsoleted one:

@available(swift, deprecated: 4.1, obsoleted: 5.0.0, message: "Swift currently only supports freeing entire heap blocks, use deallocate() instead")

Copy link
Member Author

Choose a reason for hiding this comment

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

did not know you could do that

@atrick
Copy link
Contributor

atrick commented Oct 17, 2017

@natecook1000 suggests using @available(swift, deprecated: 4.1, obsoleted: 5.0.0, message: "Swift currently only supports freeing entire heap blocks, use deallocate() instead")

That makes sense to me. @jrose-apple, is that right?

As long as we have compatibility shims for all the source breaking changes annotated with this attribute, then I'm ready to approve the changes. I think @airspeedswift also needs to approve the recent changes.

@@ -746,6 +747,7 @@ UnsafeMutable${'Raw' if IsRaw else ''}BufferPointerTestSuite.test("subscript/${R
defer { deallocateFor${Raw}Buffer(
sliceMemory, count: replacementValues.count) }

// this produces a spurious compiler warning,, someone take a look lol?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor point, but can we clean up this comment before merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

for some reason if the buffer on the line below is declared var the compiler complains that it’s never mutated, but if it’s declared let it doesn’t compile since a mutating member gets called on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

it’s out of the scope of this proposal but it’s something we should come back to later and sort out

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure; I was only talking about capitalization, punctuation, etc :)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@jrose-apple
Copy link
Contributor

@natecook1000 suggests using @available(swift, deprecated: 4.1, obsoleted: 5.0.0, message: "Swift currently only supports freeing entire heap blocks, use deallocate() instead")

That makes sense to me. @jrose-apple, is that right?

That sounds good, although the grammarian in me says that the message should use a semicolon instead of a comma.

@tayloraswift
Copy link
Member Author

tayloraswift commented Oct 18, 2017

the fangirl in me says that the message should use two commas in a row instead of one comma so compromise? 😂

@jrose-apple
Copy link
Contributor

Not going to block the PR over it, for sure. :-)

Copy link
Member

@airspeedswift airspeedswift left a comment

Choose a reason for hiding this comment

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

LGTM. Just one comment about a comment.

///
/// - Parameter capacity: The amount of memory to deallocate, counted in
/// instances of `Pointee`.
/// This pointer must be a pointer to a previously allocated memory block.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "to the start" of a previously allocated memory block, to be completely explicit.

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.

I did not realize this PR was still open. I was only waiting for @airspeedswift's review of the @available annotations. There's nothing holding this up except for the rebasing work.

…rop all parameters from deallocation, adjust namings, and add repeated-value assignment methods
@tayloraswift
Copy link
Member Author

tayloraswift commented Nov 7, 2017

this and the swift-corelibs-foundation patch should be fixed now

@tayloraswift
Copy link
Member Author

any movement on this?

@atrick
Copy link
Contributor

atrick commented Nov 15, 2017

I think anyone can merge this. If you want me to do it, I'll have time this Friday to watch the bots and deal with any fallout.

@tayloraswift
Copy link
Member Author

i mean besides xiaodi i think im the only one here who can’t commit so someone besides me is going to have to lol

@xwu
Copy link
Collaborator

xwu commented Nov 15, 2017

Hehe, one day maybe, I'll get around to requesting commit permissions, but I'm in no hurry to take on that kind of responsibility :)

@atrick
Copy link
Contributor

atrick commented Nov 17, 2017

@swift-ci test.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8dabca1

@tayloraswift
Copy link
Member Author

i really don’t know what just happened. the ci was building it fine before…

@atrick
Copy link
Contributor

atrick commented Nov 18, 2017

Please test with the following PR:

swiftlang/swift-corelibs-foundation#1247

@swift-ci Please clean test

@atrick
Copy link
Contributor

atrick commented Nov 18, 2017

The test failure is not related to this PR. All PR testing is broken:
Failure: SILOptimizer/specialize_no_definition.sil

@atrick
Copy link
Contributor

atrick commented Nov 18, 2017

Hopefully Arnold fixed the problem:
#12998

@atrick
Copy link
Contributor

atrick commented Nov 18, 2017

Please test with the following PR:

swiftlang/swift-corelibs-foundation#1247

@swift-ci Please clean test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8dabca1

@atrick
Copy link
Contributor

atrick commented Nov 18, 2017

Please test with the following PR:

swiftlang/swift-corelibs-foundation#1247

@swift-ci Please clean test

@atrick atrick merged commit c858808 into swiftlang:master Nov 18, 2017
@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8dabca1

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 8dabca1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.