[Pitch] Eliminate `ManagedProtoBuffer`


(Erik Eckstein) #1

The reason why `ManagedProtoBuffer` (the base class of `ManagedBuffer`) exists is to give the users an extra bit of type safety inside of the closure `initialHeader` passed to `ManagedBuffer.create()`.

public class ManagedBuffer<Header, Element>
  : ManagedProtoBuffer<Header, Element> {

  /// Create a new instance of the most-derived class, calling
  /// `initialHeader` on the partially-constructed object to
  /// generate an initial `Header`.
  public final class func create(
    minimumCapacity: Int,
    initialHeader: @noescape (ManagedProtoBuffer<Header, Element>) throws -> Header
  ) rethrows -> ManagedBuffer<Header, Element> {
    // ...
  }
}

This closure receives the `ManagedBuffer` instance and returns the initial value that is stored in the buffer (the header part of the buffer). We are passing the `ManagedBuffer` as `ManagedProtoBuffer` to prevent the closure from reading the uninitialized `value` property. This property is defined in `ManagedBuffer` but not in `ManagedProtoBuffer`.

  public final var header: Header {
    // ...
  }

This extra bit of safety requires the existence of `ManagedProtoBuffer`, which complicates the API.
The name may also lead to some confusion with Google's Protocol Buffers project.

This proposal suggests removing `ManagedProtoBuffer` in order to simplify the API.
It means that `ManagedBuffer` would not be derived from `ManagedProtoBuffer` and all methods from `ManagedProtoBuffer` would be moved to `ManagedBuffer`.
The closure `initialHeader` would receive a `ManagedBuffer` instead of a `ManagedProtoBuffer`.

public class ManagedBuffer<Header, Element> {

  /// Create a new instance of the most-derived class, calling
  /// `initialHeader` on the partially-constructed object to
  /// generate an initial `Header`.
  public final class func create(
    minimumCapacity: Int,
    initialHeader: @noescape (ManagedBuffer<Header, Element>) throws -> Header
  ) rethrows -> ManagedBuffer<Header, Element> {
    // ...
  }
}

Also `ManagedBufferPointer`'s init function (the second occurrence of `ManagedProtoBuffer`) would receive a `ManagedBuffer` instead of a `ManagedProtoBuffer`:

  /// Manage the given `buffer`.
  ///
  /// - Note: It is an error to use the `header` property of the resulting
  /// instance unless it has been initialized.
  internal init(_ buffer: ManagedBuffer<Header, Element>) {
    _nativeBuffer = Builtin.castToNativeObject(buffer)
  }

Motivation

···

==========
The removal of `ManagedProtoBuffer` would simplify the API and avoids confusion with Google's Protocol Buffers.

Alternatives

*) Leave as is.
*) Rename the class to avoid the "confusion"-problem.


(Brent Royal-Gordon) #2

The reason why `ManagedProtoBuffer` (the base class of `ManagedBuffer`) exists is to give the users an extra bit of type safety inside of the closure `initialHeader` passed to `ManagedBuffer.create()`.

...

This closure receives the `ManagedBuffer` instance and returns the initial value that is stored in the buffer (the header part of the buffer). We are passing the `ManagedBuffer` as `ManagedProtoBuffer` to prevent the closure from reading the uninitialized `value` property. This property is defined in `ManagedBuffer` but not in `ManagedProtoBuffer`.

...

This proposal suggests removing `ManagedProtoBuffer` in order to simplify the API.
It means that `ManagedBuffer` would not be derived from `ManagedProtoBuffer` and all methods from `ManagedProtoBuffer` would be moved to `ManagedBuffer`.
The closure `initialHeader` would receive a `ManagedBuffer` instead of a `ManagedProtoBuffer`.

I haven't used `ManagedBuffer`, but would it make sense to change the signature of `initialHeader` to `@noescape (elements: UnsafeMutablePointer<Element>, capacity: Int) -> Header` and then effectively run it inside a `withUnsafeMutablePointerToElements()` call? That would prevent access to the uninitialized `header` field while also allowing us to eliminate the `ManagedProtoBuffer` type.

···

On Jul 18, 2016, at 4:13 PM, Erik Eckstein via swift-evolution <swift-evolution@swift.org> wrote:

--
Brent Royal-Gordon
Architechies


(Charlie Monroe) #3

This is now part of a pull - https://github.com/apple/swift-evolution/pull/437

The discussion for the four items in the pull ran here (http://thread.gmane.org/gmane.comp.lang.swift.evolution/23093), though there was not enough feedback. But given the deadline for breaking changes, I decided to put the proposal together anyway.

···

On Jul 19, 2016, at 4:03 AM, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

On Jul 18, 2016, at 4:13 PM, Erik Eckstein via swift-evolution <swift-evolution@swift.org> wrote:

The reason why `ManagedProtoBuffer` (the base class of `ManagedBuffer`) exists is to give the users an extra bit of type safety inside of the closure `initialHeader` passed to `ManagedBuffer.create()`.

...

This closure receives the `ManagedBuffer` instance and returns the initial value that is stored in the buffer (the header part of the buffer). We are passing the `ManagedBuffer` as `ManagedProtoBuffer` to prevent the closure from reading the uninitialized `value` property. This property is defined in `ManagedBuffer` but not in `ManagedProtoBuffer`.

...

This proposal suggests removing `ManagedProtoBuffer` in order to simplify the API.
It means that `ManagedBuffer` would not be derived from `ManagedProtoBuffer` and all methods from `ManagedProtoBuffer` would be moved to `ManagedBuffer`.
The closure `initialHeader` would receive a `ManagedBuffer` instead of a `ManagedProtoBuffer`.

I haven't used `ManagedBuffer`, but would it make sense to change the signature of `initialHeader` to `@noescape (elements: UnsafeMutablePointer<Element>, capacity: Int) -> Header` and then effectively run it inside a `withUnsafeMutablePointerToElements()` call? That would prevent access to the uninitialized `header` field while also allowing us to eliminate the `ManagedProtoBuffer` type.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Charlie Monroe) #4

The reason why `ManagedProtoBuffer` (the base class of `ManagedBuffer`) exists is to give the users an extra bit of type safety inside of the closure `initialHeader` passed to `ManagedBuffer.create()`.

...

This closure receives the `ManagedBuffer` instance and returns the initial value that is stored in the buffer (the header part of the buffer). We are passing the `ManagedBuffer` as `ManagedProtoBuffer` to prevent the closure from reading the uninitialized `value` property. This property is defined in `ManagedBuffer` but not in `ManagedProtoBuffer`.

...

This proposal suggests removing `ManagedProtoBuffer` in order to simplify the API.
It means that `ManagedBuffer` would not be derived from `ManagedProtoBuffer` and all methods from `ManagedProtoBuffer` would be moved to `ManagedBuffer`.
The closure `initialHeader` would receive a `ManagedBuffer` instead of a `ManagedProtoBuffer`.

I haven't used `ManagedBuffer`, but would it make sense to change the signature of `initialHeader` to `@noescape (elements: UnsafeMutablePointer<Element>, capacity: Int) -> Header` and then effectively run it inside a `withUnsafeMutablePointerToElements()` call? That would prevent access to the uninitialized `header` field while also allowing us to eliminate the `ManagedProtoBuffer` type.

Wouldn't this disallow access to the capacity field? That's as well defined on ManagedProtoBuffer and AFAIK can be accessed during the initialization.

···

On Jul 19, 2016, at 4:03 AM, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

On Jul 18, 2016, at 4:13 PM, Erik Eckstein via swift-evolution <swift-evolution@swift.org> wrote:

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Brent Royal-Gordon) #5

Yes, which is why I suggested it should be passed in to the `initialHeader` closure too. (It's read-only anyway, so there's no loss of capability.) What it *does* prevent access to is `withUnsafeMutablePointerToHeader`, but I'm not sure how that method is supposed to work before the header's been initialized anyway.

···

On Jul 18, 2016, at 10:05 PM, Charlie Monroe <charlie@charliemonroe.net> wrote:

I haven't used `ManagedBuffer`, but would it make sense to change the signature of `initialHeader` to `@noescape (elements: UnsafeMutablePointer<Element>, capacity: Int) -> Header` and then effectively run it inside a `withUnsafeMutablePointerToElements()` call? That would prevent access to the uninitialized `header` field while also allowing us to eliminate the `ManagedProtoBuffer` type.

Wouldn't this disallow access to the capacity field? That's as well defined on ManagedProtoBuffer and AFAIK can be accessed during the initialization.

--
Brent Royal-Gordon
Architechies


(Charlie Monroe) #6

I haven't used `ManagedBuffer`, but would it make sense to change the signature of `initialHeader` to `@noescape (elements: UnsafeMutablePointer<Element>, capacity: Int) -> Header` and then effectively run it inside a `withUnsafeMutablePointerToElements()` call? That would prevent access to the uninitialized `header` field while also allowing us to eliminate the `ManagedProtoBuffer` type.

Wouldn't this disallow access to the capacity field? That's as well defined on ManagedProtoBuffer and AFAIK can be accessed during the initialization.

Yes, which is why I suggested it should be passed in to the `initialHeader` closure too. (It's read-only anyway, so there's no loss of capability.) What it *does* prevent access to is `withUnsafeMutablePointerToHeader`, but I'm not sure how that method is supposed to work before the header's been initialized anyway.

I'd personally just remove the ManagedProtoBuffer and note in the documentation of ManagedBuffer.create that you should not read the header from within the block. I think it's a similar programming error as indexing out of bounds which is also documented to be a programming error.

I'm not sure whether we could simply eliminate ManagedBuffer altogether since looking at https://github.com/search?l=swift&q=ManagedBuffer&type=Code the only places this is used are generally just forks of apple/swift and I haven't found a single use ManagedBuffer.create in the entire stdlib, all of the Array code uses ManagedBufferPointer directly and initializes it with the unsafeBufferObject, which is some subclass of _ContiguousArrayStorageBase...

···

On Jul 19, 2016, at 8:49 AM, Brent Royal-Gordon <brent@architechies.com> wrote:

On Jul 18, 2016, at 10:05 PM, Charlie Monroe <charlie@charliemonroe.net> wrote:

--
Brent Royal-Gordon
Architechies


(Erik Eckstein) #7

This is now part of a pull - https://github.com/apple/swift-evolution/pull/437

The discussion for the four items in the pull ran here (http://thread.gmane.org/gmane.comp.lang.swift.evolution/23093), though there was not enough feedback. But given the deadline for breaking changes, I decided to put the proposal together anyway.

OK. How do you like to proceed with that? I'm fine to handle this as part of #437 as long as it goes through quickly. If it's accepted, do you want to do the implementation? Otherwise I can do it (the removal of ManagedProtoBuffer). Roman could help with the `unsafeAddressOf` part.

I haven't used `ManagedBuffer`, but would it make sense to change the signature of `initialHeader` to `@noescape (elements: UnsafeMutablePointer<Element>, capacity: Int) -> Header` and then effectively run it inside a `withUnsafeMutablePointerToElements()` call? That would prevent access to the uninitialized `header` field while also allowing us to eliminate the `ManagedProtoBuffer` type.

Wouldn't this disallow access to the capacity field? That's as well defined on ManagedProtoBuffer and AFAIK can be accessed during the initialization.

Yes, which is why I suggested it should be passed in to the `initialHeader` closure too. (It's read-only anyway, so there's no loss of capability.) What it *does* prevent access to is `withUnsafeMutablePointerToHeader`, but I'm not sure how that method is supposed to work before the header's been initialized anyway.

I'd personally just remove the ManagedProtoBuffer and note in the documentation of ManagedBuffer.create that you should not read the header from within the block. I think it's a similar programming error as indexing out of bounds which is also documented to be a programming error.

I agree.

···

On Jul 19, 2016, at 12:17 AM, Charlie Monroe <charlie@charliemonroe.net> wrote:

On Jul 19, 2016, at 8:49 AM, Brent Royal-Gordon <brent@architechies.com> wrote:

On Jul 18, 2016, at 10:05 PM, Charlie Monroe <charlie@charliemonroe.net> wrote:

I'm not sure whether we could simply eliminate ManagedBuffer altogether since looking at https://github.com/search?l=swift&q=ManagedBuffer&type=Code the only places this is used are generally just forks of apple/swift and I haven't found a single use ManagedBuffer.create in the entire stdlib, all of the Array code uses ManagedBufferPointer directly and initializes it with the unsafeBufferObject, which is some subclass of _ContiguousArrayStorageBase...

--
Brent Royal-Gordon
Architechies