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

[stdlib] Restore MemoryLayout.*(ofValue:) #4041

Merged
merged 3 commits into from Aug 12, 2016
Merged

[stdlib] Restore MemoryLayout.*(ofValue:) #4041

merged 3 commits into from Aug 12, 2016

Conversation

xwu
Copy link
Collaborator

@xwu xwu commented Aug 5, 2016

What's in this pull request?

This PR, requested by Dave Abrahams, restores ofValue variants of size, stride, and alignment.

I removed the hardcoded fix-its for migrating away from sizeofValue(_:) and friends, since the restoration of these counterparts means that a simple renamed will do.

I also took the liberty of a little gardening on the doc comments (80-character columns, etc.).


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library only for macOS. Simulator standard libraries and
    device standard libraries are not built.
  3. lldb is not built.
  4. The test and validation-test targets are run only for macOS. The optimized
    version of these tests are not run.

A smoke test on Linux does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library incrementally.
  3. lldb is built incrementally.
  4. The swift test and validation-test targets are run. The optimized version of these
    tests are not run.
  5. lldb is tested.

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@tkremenek
Copy link
Member

@swift-ci test

@@ -284,9 +284,6 @@ func _MemoryLayout<T>(t: T) {
_ = sizeof(T.self) // expected-error {{'sizeof' is unavailable: use MemoryLayout<T>.size instead.}} {{7-14=MemoryLayout<}} {{15-21=>.size}} {{none}}
_ = alignof(T.self) // expected-error {{'alignof' is unavailable: use MemoryLayout<T>.alignment instead.}} {{7-15=MemoryLayout<}} {{16-22=>.alignment}} {{none}}
_ = strideof(T.self) // expected-error {{'strideof' is unavailable: use MemoryLayout<T>.stride instead.}} {{7-16=MemoryLayout<}} {{17-23=>.stride}} {{none}}
_ = sizeofValue(t) // expected-error {{'sizeofValue' is unavailable: use MemoryLayout<T>.size instead.}} {{7-21=MemoryLayout<T>.size}} {{none}}
_ = alignofValue(t) // expected-error {{'alignofValue' is unavailable: use MemoryLayout<T>.alignment instead.}} {{7-22=MemoryLayout<T>.alignment}} {{none}}
_ = strideofValue(t) // expected-error {{'strideofValue' is unavailable: use MemoryLayout<T>.stride instead.}} {{7-23=MemoryLayout<T>.stride}} {{none}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add tests for the new migration fixits that you added?

Copy link
Collaborator Author

@xwu xwu Aug 5, 2016

Choose a reason for hiding this comment

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

Will do, as soon as I'm in front of a computer this afternoon.

Edit: done.

@tkremenek
Copy link
Member

@swift-ci test

@dabrahams
Copy link
Collaborator

@swift-ci Please smoke test

@dabrahams
Copy link
Collaborator

@swift-ci Please smoke test Linux platform

@xwu
Copy link
Collaborator Author

xwu commented Aug 6, 2016

@dabrahams Does the Linux failure look related to you?

@dabrahams
Copy link
Collaborator

@swift-ci Please smoke test Linux platform

@dabrahams
Copy link
Collaborator

on Fri Aug 05 2016, Xiaodi Wu <notifications-AT-github.com> wrote:

@dabrahams Does the Linux failure look related to you?

I can imagine one way they might be related, but it seems like a
stretch. re-testing.

-Dave

/// Returns the contiguous memory footprint of `T`.
///
/// Does not include any dynamically-allocated or "remote" storage. In
/// particular, `MemoryLayout(ofValue: x).size`, when `x` is a class instance,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo in documentation comment: MemoryLayout(ofValue: x).size => MemoryLayout.size(ofValue: x)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Nate Cook has just cleaned up the documentation for the existing bits of MemoryLayout quite a bit, so I'll either rebase or submit a follow-up patch and incorporate a correction.

@xwu
Copy link
Collaborator Author

xwu commented Aug 7, 2016

@dabrahams Second thoughts on kicking off the review?

@dabrahams
Copy link
Collaborator

@xwu No, darnit. I was having technical issues with my email 😣. Should have gone out now.

@xwu
Copy link
Collaborator Author

xwu commented Aug 8, 2016

(Rebased.)

@dabrahams
Copy link
Collaborator

@xwu rebasing doesn't really help unless someone added a merge conflict upstrea. It just means we need to test again

@dabrahams
Copy link
Collaborator

@swift-ci Please test

@xwu
Copy link
Collaborator Author

xwu commented Aug 8, 2016

I'm aware. Nate Cook did a massive doc comment update that included MemoryLayout; this was not an optional rebase.

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2016

@swift-ci please smoke test OS X platform.

@dabrahams dabrahams added swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process and removed swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review labels Aug 10, 2016
@dabrahams dabrahams merged commit e6dec58 into apple:master Aug 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants