Skip to content
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

Add ArrayStorage. #23

Merged
merged 2 commits into from
May 20, 2020
Merged

Add ArrayStorage. #23

merged 2 commits into from
May 20, 2020

Conversation

dabrahams
Copy link
Collaborator

@dabrahams dabrahams commented May 20, 2020

No description provided.

Copy link
Owner

@saeta saeta left a comment

Choose a reason for hiding this comment

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

Approving to unblock progress; have a few questions / comments. Thanks for this @dabrahams !!

Sources/PenguinStructures/ArrayStorage.swift Show resolved Hide resolved
if r == capacity { return nil }
access.withUnsafeMutablePointers { h, e in
(e + r).initialize(to: x)
h[0].count = r + 1
Copy link
Owner

Choose a reason for hiding this comment

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

Is this the same as:

Suggested change
h[0].count = r + 1
h.pointee.count = r + 1

Or am I missing something? If so, I think the suggestion might be easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they're the same. I don't have a strong opinion about which is easier to read; they each have strengths. If you have any conviction about this, I'll take your suggestion. But I wish I could write h*.count. And maybe we should have that operator in penguin internals.

Copy link
Owner

Choose a reason for hiding this comment

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

+1 for having an operator. I can put together a PR shortly, and I'll update this code as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could follow Pascal precedent and have h^.count. I think I prefer * tho.

Sources/PenguinStructures/ArrayStorage.swift Show resolved Hide resolved
///
/// This protocol's extensions provide APIs that depend on the element type, and
/// the implementations for `AnyArrayStorage` APIs.
public protocol ArrayStorageImplementation: AnyArrayStorageImplementation {
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: in Swift, I associate Arrays with value semantics, but I didn't see CoW machinery here... would it be better to call this BufferStorageImplementation or maybe BufferImplementation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

“Array” has the connotations I was looking for: contiguity and homogeneity. “Buffer” doesn't necessarily do that. Also, this follows the convention set in the standard library: “Storage” is a class that provides the storage (https://github.com/apple/swift/blob/master/stdlib/public/core/ContiguousArrayBuffer.swift#L71), “ContiguousArrayBuffer” is a struct that adds resizability, and “Array” wraps that to provide value semantics.

Happy to use some other terms if you're convinced they're better though.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with following the precedent for now, and we can think harder about this over time. LGTM!

@saeta saeta merged commit 100a756 into master May 20, 2020
@saeta saeta deleted the array-storage branch May 20, 2020 21:46
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.

2 participants