[SE-0101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct - Part 1 #3854

rintaro · 2016-07-29T05:36:05Z

Currently sizeof family functions are simply marked as deprecated.

Merge this PR.
Merge PRs to other repos.
- [SE-0101] Migration swift-corelibs-foundation#491
- [SE-0101] Migration swift-corelibs-libdispatch#125
- [SE-0101] Migration swift-package-manager#571
- [Do not merge yet][SE-0101] Migration apple/swift-lldb#38
- ~~swift-corelibs-xctest~~ no need
- ~~swift-xcode-playground-support~~ only in comments.
Mark obsoleted functions as unavailable, and turn on migration fix-it.

Still need more test case for migration support.

CC:
@tkremenek
Are you still accepting new implementation?

@gribozavr
Could you review stdlib part? Added stdlib/public/core/MemoryLayout.swift
@jrose-apple, @DougGregor
~~Could you review fix-it part?~~ Removed from this PR. I will split them into another PR.

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:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
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:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
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.

gribozavr · 2016-07-29T05:49:13Z

@rintaro To get CI coverage and avoid lockstep issues, you could add old declarations as deprecated. Then we can independently land fixes to swift-corelibs-* and then remove the deprecated syntax. What do you think?

rintaro · 2016-07-29T05:55:11Z

@gribozavr OK, I'll try it.

gribozavr · 2016-07-29T05:59:24Z

stdlib/public/SDK/Dispatch/Data.swift

@@ -114,7 +114,7 @@ public struct DispatchData : RandomAccessCollection, _ObjectiveCBridgeable {
 	///
 	/// - parameter buffer: The buffer of bytes to append. The size is calculated from `SourceType` and `buffer.count`.
 	public mutating func append<SourceType>(_ buffer : UnsafeBufferPointer<SourceType>) {
-		self.append(UnsafePointer(buffer.baseAddress!), count: buffer.count * sizeof(SourceType.self))
+		self.append(UnsafePointer(buffer.baseAddress!), count: buffer.count * MemoryLayout<SourceType>.size)


This code was changed in the master branch, would you mind rebasing?

Also, could you change sizeof to strideof? Using sizeof here can cause bugs.

gribozavr · 2016-07-29T06:11:42Z

stdlib/public/core/MemoryLayout.swift

+  public static func of(_ candidate : @autoclosure () -> T) -> MemoryLayout<T>.Type {
+    return MemoryLayout.init(candidate).dynamicType
+  }
+}


I think we can implement it simpler:

extension MemoryLayout { public static func of(_ candidate: @autoclosure () -> T) -> MemoryLayout<T>.Type { return MemoryLayout<T>.self } }

I just copy pasted it from the proposal. 😓
I'll test it.

gribozavr · 2016-07-29T06:22:34Z

@rintaro Thank you! Let's see if we can get a CI run:

@swift-ci Please test OS X platform

gribozavr · 2016-07-29T06:23:09Z

I reviewed the patch, LGTM as written, but I will do another pass after you rebase.

rudkx · 2016-07-29T06:36:54Z

MiscDiagnostics.cpp changes LGTM!

rintaro · 2016-07-29T07:16:47Z

stdlib/public/SDK/Dispatch/Data.swift

-		buffer.baseAddress!.withMemoryRebound(to: UInt8.self, capacity: buffer.count * strideof(SourceType.self)) {
-			self.append($0, count: buffer.count * sizeof(SourceType.self))
+		buffer.baseAddress!.withMemoryRebound(to: UInt8.self, capacity: buffer.count * MemoryLayout<SourceType>.stride) {
+			self.append($0, count: buffer.count * MemoryLayout<SourceType>.stride)


@gribozavr You didn't mention this sizeof -> stride change. This should be .size?
This was introduced in recent change.(https://github.com/apple/swift/blob/0b75ee975e55ffa7c8a69a0f076f33ff82b64f44/stdlib/public/SDK/Dispatch/Data.swift#L118) @atrick ?

This is supposed to be .stride on both modified lines.
I forgot to change the original code to strideof() and hadn't pushed my fix yet.

@atrick
Thank you! If you have more fix, feel free to push them first.
I'd happy to rebase on it.

I won't push my fix, it makes more sense as part of your PR. Thanks.

gribozavr · 2016-07-29T07:49:34Z

@swift-ci Please test

xwu · 2016-07-29T08:16:12Z

IIUC, the core team decision for SE-0101 was that MemoryLayout should be implemented as an enum with no cases. I imagine that this also means the ofValue variants are not to be included (a slightly bigger migration task).

rintaro · 2016-07-29T08:22:48Z

@xwu Sorry, I didn't read the Rationale, and the proposal itself carefully..
I'll read the evolution thread later. But, sadly, I'm busy for a few hours from now :(

rintaro · 2016-07-29T10:52:41Z

As for struct => enum, no problem.
As for .of(value), I think we can remove it.
I defer the decision to the core team. @gribozavr, @dabrahams ?

If we decide to drop it, the implementation of migration fix-it cannot be fully implemented, or will be very complicated.

Contrived example here:

func fn<S: Sequence>(s: S) {
  var i = s.makeIterator()
  let size = sizeofValue(i.next()!)
  // Compare:
  // let size = MemoryLayout<S.Iterator.Element>.size; _ = i.next()!
  // let size = MemoryLayout.of(i.next()!).size

We have to actually look into the type of the value.
And, in case of reading the value has side effects, we have to evaluate and discard the result.
I assume we can implement that, but I'm not sure I can.
And typically, we prefer minimally modify the source (by fix-it).

rintaro · 2016-07-29T11:07:29Z

stdlib/public/core/MemoryLayout.swift

+
+extension MemoryLayout {
+  @_transparent
+  public static func of(_ candidate : @autoclosure () -> T) -> MemoryLayout<T>.Type {


candidate is not needed here.

gribozavr · 2016-07-29T17:38:51Z

@rintaro Thanks again for the implementation. I just checked with @dabrahams and he said that the core team did not approve the .of(_:) API. But, seeing the uses of it in the library, we think it would be a regression for our code to remove it, so we ask you to to keep it as a public @testable function with the name MemoryLayout._ofInstance(_:), and maybe re-open the discussion about adding it back as a public API.

You should keep the uses of this underscored _ofInstance() function in the swift.git repo, but eliminate the uses of it from other repos (hopefully, there are not so many of them as in the primary repo).

Does this sound good?

Sorry for not providing this feedback earlier!

rintaro · 2016-07-29T17:44:46Z

@gribozavr no problem 👍 I'll update the PR soon.

rintaro · 2016-07-29T17:47:23Z

@gribozavr Can I split the changes to lib/Sema/MiscDiagnostics.cpp into another PR?
I think that should be applied later.

gribozavr · 2016-07-29T17:47:49Z

@rintaro Absolutely!

@Private

…MemoryLayout struct As of now: * old APIs are just marked as `deprecated` not `unavaiable`. To make it easier to co-operate with other toolchain repos. * Value variant of API is implemented as public @Private `_ofInstance(_:)`.

rintaro · 2016-07-29T18:17:21Z

@gribozavr Updated.

rintaro · 2016-07-29T18:35:45Z

@swift-ci Please smoke test

xwu · 2016-07-29T18:52:37Z

Since _ofInstance is internal API, and as you point out @autoclosure actually changes the semantics, perhaps safest to restore the original ofValue semantics for _ofInstance so that all side effects are still triggered?

rintaro · 2016-07-29T19:32:47Z

Since all of current usages of _ofInstance(_:) in this repo has no side effects, we can keep @autoclosure. But I don't know which is faster. Let's try

@swift-ci Please benchmark

swift-ci · 2016-07-29T20:21:22Z

Build comment file:

gribozavr · 2016-07-29T20:54:45Z

@rintaro There should be no difference with and without autoclosure. I will look at the generated code right now.

gribozavr · 2016-07-29T20:57:20Z

stdlib/public/SDK/Dispatch/Data.swift

@@ -149,7 +149,7 @@ public struct DispatchData : RandomAccessCollection, _ObjectiveCBridgeable {

 	/// Copy the contents of the data into a buffer.
 	///
-	/// This function copies the bytes in `range` from the data into the buffer. If the count of the `range` is greater than `sizeof(DestinationType) * buffer.count` then the first N bytes will be copied into the buffer.
+	/// This function copies the bytes in `range` from the data into the buffer. If the count of the `range` is greater than `MemoryLayout<DestinationType>.size * buffer.count` then the first N bytes will be copied into the buffer.


.stride. Please make this change as a separate commit so that it does not invalidate the CI results. (This is comment-only, so it does not matter for the purposes of CI.)

gribozavr · 2016-07-29T21:00:24Z

@rintaro The code as it is now, with the autoclosure, is optimal. The optimizer eliminates the closure. Let's keep the autoclosure.

swift-ci · 2016-07-29T21:14:49Z

Build comment file:

gribozavr · 2016-07-29T21:52:15Z

@swift-ci Please test

gribozavr · 2016-07-29T21:54:07Z

The changes look good to me, but I want to get a full CI run, with all the platforms and downstream projects.

@rintaro Please submit the C++ code changes and the comment change I requested as separate pull requests, thank you!

gribozavr · 2016-07-29T22:56:24Z

Linux failures are unrelated. Merging.

rintaro · 2016-08-02T14:43:40Z

@gribozavr Can I ask you to update CHANGELOG.md?
Because I'm not good at writing in English.

rintaro changed the title ~~[WIP][SE-101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct~~ [WIP][SE-0101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct Jul 29, 2016

rintaro force-pushed the SE-0101-memorylayout branch from 373e7ff to 924a504 Compare July 29, 2016 05:42

gribozavr reviewed Jul 29, 2016
View reviewed changes

rintaro force-pushed the SE-0101-memorylayout branch from 924a504 to 373e7ff Compare July 29, 2016 06:09

gribozavr reviewed Jul 29, 2016
View reviewed changes

jessesquires mentioned this pull request Jul 29, 2016

Issue 33 - after Swift 3 SwiftWeekly/swiftweekly.github.io#57

Closed

rintaro force-pushed the SE-0101-memorylayout branch from 373e7ff to 2128555 Compare July 29, 2016 07:08

rintaro reviewed Jul 29, 2016
View reviewed changes

rintaro force-pushed the SE-0101-memorylayout branch from 2128555 to ae977ab Compare July 29, 2016 07:38

rintaro reviewed Jul 29, 2016
View reviewed changes

rintaro added 3 commits July 30, 2016 03:09

[SE-0101] MemoryLayout: Migrate testsuite and benchmarks

c6f4bcd

[SE-0101] .size to .stride

06603c1

rintaro force-pushed the SE-0101-memorylayout branch from 9e9149f to 06603c1 Compare July 29, 2016 18:11

rintaro changed the title ~~[WIP][SE-0101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct~~ [SE-0101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct Jul 29, 2016

rintaro changed the title ~~[SE-0101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct~~ [SE-0101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct - Part 1 Jul 29, 2016

gribozavr reviewed Jul 29, 2016
View reviewed changes

gribozavr merged commit 99dffd7 into swiftlang:master Jul 29, 2016

This was referenced Jul 30, 2016

[SE-0101] Migrate sizeof() to MemoryLayout swiftlang/swift-package-manager#578

Merged

[SE-0101] Migration apple/swift-lldb#39

Merged

rintaro mentioned this pull request Jul 30, 2016

[SE-0101] Migration support fix-it #3883

Merged

1 task

rintaro deleted the SE-0101-memorylayout branch August 1, 2016 07:04

rintaro mentioned this pull request Aug 1, 2016

[SE-0101] Migration swiftlang/swift-corelibs-libdispatch#129

Merged

jessesquires mentioned this pull request Aug 4, 2016

Update status for SE-0101, SE-0103, SE-0117 swiftlang/swift-evolution#487

Merged

[SE-0101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct - Part 1 #3854

[SE-0101] Implement: Reconfiguring sizeof and related functions into a unified MemoryLayout struct - Part 1 #3854

Uh oh!

Conversation

rintaro commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribozavr commented Jul 29, 2016

Uh oh!

rintaro commented Jul 29, 2016

Uh oh!

gribozavr Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

gribozavr Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

rintaro Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

rintaro Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jul 29, 2016

Uh oh!

gribozavr commented Jul 29, 2016

Uh oh!

rudkx commented Jul 29, 2016

Uh oh!

rintaro Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

rintaro Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jul 29, 2016

Uh oh!

xwu commented Jul 29, 2016

Uh oh!

rintaro commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jul 29, 2016

Uh oh!

rintaro commented Jul 29, 2016

Uh oh!

rintaro commented Jul 29, 2016

Uh oh!

gribozavr commented Jul 29, 2016

Uh oh!

rintaro commented Jul 29, 2016

Uh oh!

rintaro commented Jul 29, 2016

Uh oh!

xwu commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Jul 29, 2016

Uh oh!

swift-ci commented Jul 29, 2016

Build comment file:

Uh oh!

gribozavr commented Jul 29, 2016

Uh oh!

gribozavr Jul 29, 2016

rintaro commented Jul 29, 2016 •

edited

Loading

rintaro Jul 29, 2016 •

edited

Loading

rintaro Jul 29, 2016 •

edited

Loading

rintaro commented Jul 29, 2016 •

edited

Loading

rintaro commented Jul 29, 2016 •

edited

Loading

rintaro Jul 29, 2016 •

edited

Loading

xwu commented Jul 29, 2016 •

edited

Loading