Skip to content

Conversation

jckarter
Copy link
Contributor

@jckarter jckarter commented Mar 26, 2018

If a key path refers to inline storage of the root type, this produces the offset in bytes between a pointer to the root and a pointer to the projected storage. Otherwise, returns nil.

Proposal: swiftlang/swift-evolution#818

…inline storage.

If a key path refers to inline storage of the root type, this produces the offset in bytes between a pointer to the root and a pointer to the projected storage. Otherwise, returns nil.
@jckarter
Copy link
Contributor Author

@swift-ci Please test

1 similar comment
@jckarter
Copy link
Contributor Author

@swift-ci Please test

@jckarter
Copy link
Contributor Author

@natecook1000 Mind reviewing my doc comments here too?

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.

Thanks—a few notes below!

/// get a pointer to the storage accessed by `key`. If the return value is
/// non-nil, then these formulations are equivalent:
///
/// var root: T, value: U
Copy link
Member

Choose a reason for hiding this comment

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

We need to have four spaces of indentation (five including the initial space after the triple-slash) for a code sample to be recognized. Within the code sample, we also use four-space indentation instead of the stdlib-standard two.

/// // Mutation through the key path...
/// root[keyPath: \.key] = value
/// // ...is exactly equivalent to mutation through the offset pointer...
/// withUnsafePointer(to: &root) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be withUnsafeMutablePointer since they're modifying an underlying value.

/// var root: T, value: U
/// var key: WritableKeyPath<T, U>
/// // Mutation through the key path...
/// root[keyPath: \.key] = value
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 you need the \. here and below, since key is a key path instance, not the path itself.

/// Returns the offset of an inline stored property of `T` within the
/// in-memory representation of `T`.
///
/// If the given `key` refers to inline storage within the
Copy link
Member

Choose a reason for hiding this comment

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

I think this initial paragraph could focus on the happy path: "If the given key refers to inline, directly addressable storage within the in-memory representation of T, the the return value…". After the example you have below, with root and value, we could use your description here of what might disqualify a property from being measured in this way, with an example type like:

struct ProductCategory {
    var name: String
    var updateCounter: Int
    var products: [Product] {
        didSet { updateCounter += 1 }
    }
    var productCount: Int {
        return products.count
    }
    var someAbstractedProperty: () -> () // I can't think of a good example here
}

And then we could point out the valid and invalid properties either in comments or in text. If I'm understanding this right, updateCounter and name would be safe, but accessing properties of name wouldn't—I don't think I included a non-inline property. What do you think?

@karwa
Copy link
Contributor

karwa commented Mar 31, 2018

Is it worth noting in the documentation that non-@fixed_contents structs reserve the right to change properties from stored to computed? If you write code which depends on a non-optional offset for a particular keypath, it might not continue to work in later versions of the library.

@jckarter
Copy link
Contributor Author

jckarter commented Apr 2, 2018

That's an important point, @karwa . I noted that in the proposal, but it may bear mentioning in the doc comment as well.

@DougGregor
Copy link
Member

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 0eb5131 into swiftlang:master May 10, 2018
@slavapestov
Copy link
Contributor

@karwa Even @fixedContents structs can change offsets if the types of the fields are themselves resilient.

@natecook1000
Copy link
Member

How does this look for that caveat?

  /// When using `offset(of:)` with a type imported from a library, don't assume
  /// that future versions of the library will have the same behavior. If a
  /// property is converted from a stored property to a computed property, the
  /// result of `offset(of:)` changes to `nil`. That kind of conversion is
  /// non-breaking in other contexts, but would trigger a runtime error if the
  /// result of `offset(of:)` is force-unwrapped.

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.

6 participants