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

Update to Swift 4.2, use Hasher, fix Xcode 10.2 compilation complaints. #157

Merged
merged 14 commits into from
Feb 27, 2019

Conversation

laposheureux
Copy link
Contributor

@laposheureux laposheureux commented Feb 5, 2019

I think I've massaged away the Travis issues and fixed the obvious compilation problems across platforms...pending Travis giving me a green light finally.

Info about Hasher here: https://github.com/apple/swift-evolution/blob/master/proposals/0206-hashable-enhancements.md

// On 32bit CPUs, this will *potentially, rarely, in the case of generating tons of tokens* be problematic.
// Token gives back an Int64 which on 32bit platforms when cast to Int could overflow. This is not a factor
// on iOS 11+ as that OS supports 64bit CPUs only and isn't a factor on macOS 10.7+.
return ModelVersion(hash: Int(Token.makeUnique().rawValue))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth calling out this issue with a comment.


public var hashValue: Int {
return sectionIndex.hashValue ^ itemIndex.hashValue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Automatic hashable conformance.

case .doubleClick:
return 1<<6
hasher.combine(1<<6)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not convinced this is the best we can do here, but it's what we had.

public var hashValue: Int {
return value.hashValue
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Automatic conformance to hashable

@@ -1422,6 +1401,7 @@
PRODUCT_BUNDLE_IDENTIFIER = com.Dropbox.Pilot;
PRODUCT_NAME = Pilot;
SKIP_INSTALL = YES;
SWIFT_VERSION = 4.2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need Swift 4.2 for Hasher

Copy link
Contributor

Choose a reason for hiding this comment

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

We try to put these in .xccodeconfig files when possible

@wkiefer
Copy link
Contributor

wkiefer commented Feb 5, 2019

Heya @danielrhammond@laposheureux and I chatted briefly offline. Two quick recommendations:

  • Maybe separate out the 10.2 beta changes from stable swift 4.2
  • Was a little worried removal of inner public access control to the public extension { public method() {} } cases would be backwards breaking. Maybe we can remove public from the extension but leave on the methods themselves

thanks!

@danielrhammond
Copy link
Contributor

This looks good to me, and checks out on 10.1, @wkiefer mind taking a look with the model version compat stuff + I think your previous comments now addressed?

Copy link
Contributor

@wkiefer wkiefer left a comment

Choose a reason for hiding this comment

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

LGTM - thx!

@@ -1422,6 +1401,7 @@
PRODUCT_BUNDLE_IDENTIFIER = com.Dropbox.Pilot;
PRODUCT_NAME = Pilot;
SKIP_INSTALL = YES;
SWIFT_VERSION = 4.2;
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to put these in .xccodeconfig files when possible


open override func validateMenuItem(_ menuItem: NSMenuItem) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an odd change — might want to sanity check it's still getting called

@laposheureux
Copy link
Contributor Author

laposheureux commented Feb 12, 2019 via email

@danielrhammond danielrhammond merged commit 3b9636b into dropbox:master Feb 27, 2019
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.

None yet

3 participants