Skip to content

Conversation

jckarter
Copy link
Contributor

@jckarter jckarter commented Mar 29, 2018

Since the functions produce pointers with tightly-scoped lifetimes there's no formal reason these have to only work on inout things. Now that arguments can be +0, we can even do this without copying values that we already have at +0.

Proposal: swiftlang/swift-evolution#822

@jckarter
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 65457d3877ae24da0683ceba0e523fa658312b8f

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.

This is great, Joe!

Copy link
Member

Choose a reason for hiding this comment

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

Lost the end of the sentence here…

Copy link
Member

Choose a reason for hiding this comment

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

This parameter is named value, and we've got lots of args above—what do you think of standardizing on value throughout? It's less jargony.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add the parameters to the function name where this is mentioned? withUnsafeMutablePointer(to:_:)

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 65457d3877ae24da0683ceba0e523fa658312b8f

@atrick
Copy link
Contributor

atrick commented Mar 30, 2018

Very nice.

@jckarter jckarter force-pushed the withUnsafePointer-for-lets branch from 65457d3 to 670903b Compare March 30, 2018 20:41
@jckarter
Copy link
Contributor Author

@swift-ci Please test

@jckarter
Copy link
Contributor Author

@natecook1000 Thanks for the feedback! How do the doc comments look now?

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 65457d3877ae24da0683ceba0e523fa658312b8f

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 65457d3877ae24da0683ceba0e523fa658312b8f

jckarter added a commit to jckarter/swift that referenced this pull request Mar 30, 2018
The resilience model forces inlinable code to use resiliently conservative access paths for properties in the same module, but some stdlib methods by design escapes storage of a global variable. The additional `Builtin.addressof` validation introduced in swiftlang#15608 exposed the latent issue here.
jckarter added a commit to jckarter/swift that referenced this pull request Mar 30, 2018
The resilience model forces inlinable code to use resiliently conservative access paths for properties in the same module, but some stdlib methods by design escapes storage of a global variable. The additional `Builtin.addressof` validation introduced in swiftlang#15608 exposed the latent issue here.
@natecook1000
Copy link
Member

This looks great, @jckarter — thanks! Good note on exclusivity concerns, too. 👍

@jckarter
Copy link
Contributor Author

@xedin Any idea why this change would have regressed diagnostics in this test in test/Constraints/diagnostics.swift?

func read<T : BinaryInteger>() -> T? {
  var buffer : T 
  let n = withUnsafePointer(to: &buffer) { (p) in
    read2(UnsafePointer(p), maxLength: MemoryLayout<T>.size) // expected-error {{cannot convert value of type 'UnsafePointer<_>' to expected argument type 'UnsafeMutableRawPointer'}}
  }
}

I would've expected no impact, since the & should still make which withUnsafePointer overload gets chosen unambiguous, but it changes the diagnostics we report, possibly due to the overload set in the solution:

******************** TEST 'Swift(macosx-x86_64) :: Constraints/diagnostics.swift' FAILED ********************
16:43:47 Script:
16:43:47 --
16:43:47 /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/bin/swiftc -frontend -target x86_64-apple-macosx10.9  -module-cache-path '/var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/swift-testsuite-clang-module-cachegSRGYq' -sdk /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk -swift-version 3  -typecheck -verify -disable-objc-attr-requires-foundation-module /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/test/Constraints/diagnostics.swift
16:43:47 --
16:43:47 Exit Code: 1
16:43:47 
16:43:47 Command Output (stderr):
16:43:47 --
16:43:47 
/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/test/Constraints/diagnostics.swift:755:11: error: unexpected note produced: explicitly specify the generic arguments to fix this issue
16:43:47     read2(UnsafePointer(p), maxLength: MemoryLayout<T>.size) // expected-error {{cannot convert value of type 'UnsafePointer<_>' to expected argument type 'UnsafeMutableRawPointer'}}
16:43:47           ^
16:43:47 /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/test/Constraints/diagnostics.swift:755:82: error: incorrect message found
16:43:47     read2(UnsafePointer(p), maxLength: MemoryLayout<T>.size) // expected-error {{cannot convert value of type 'UnsafePointer<_>' to expected argument type 'UnsafeMutableRawPointer'}}
16:43:47                                                                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
16:43:47                                                                                  generic parameter 'Pointee' could not be inferred
16:43:47 
16:43:47 --
16:43:47 
16:43:47 ********************

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure that's no good either, because we want this to fold down to a single address reference. Did we ever implement the thing where _emptyStringStorage was itself inlinable, meaning that you could rely on it being stored?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. We're supposed to fold all the va_list-building down to primitive ops.

@jckarter jckarter force-pushed the withUnsafePointer-for-lets branch from 670903b to a3536a6 Compare April 18, 2018 02:49
@jckarter
Copy link
Contributor Author

@swift-ci Please test

@jckarter
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 670903b39ede9b42ab713e4524fbf783ecb148d0

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 670903b39ede9b42ab713e4524fbf783ecb148d0

@jckarter jckarter force-pushed the withUnsafePointer-for-lets branch from a3536a6 to 262137d Compare April 18, 2018 16:13
…able arguments.

Since the functions produce pointers with tightly-scoped lifetimes there's no formal reason these have to only work on `inout` things.  Now that arguments can be +0, we can even do this without copying values that we already have at +0.
@jckarter jckarter force-pushed the withUnsafePointer-for-lets branch from 262137d to 62771a0 Compare April 18, 2018 16:14
@jckarter
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - a3536a6a541ba93f2b3dbeefe3d41317d6bf4877

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - a3536a6a541ba93f2b3dbeefe3d41317d6bf4877

@jckarter jckarter merged commit 0ee381e into swiftlang:master Apr 18, 2018
@jckarter jckarter changed the title [pending] stdlib: Add withUnsafeBytes(of:) and withUnsafePointer(to:) for immutable arguments. stdlib: Add withUnsafeBytes(of:) and withUnsafePointer(to:) for immutable arguments. Apr 18, 2018
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.

5 participants