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

Swift 4.1 #228

Merged
merged 9 commits into from
Sep 10, 2018
Merged

Swift 4.1 #228

merged 9 commits into from
Sep 10, 2018

Conversation

yonaskolb
Copy link
Collaborator

This updates the codebase to Swift 4.1 and drops Swift 3 support

Warnings were not cleaned up in Lexer.swift as #226 will take care of that

Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Only reviewed this on GitHub and not in Xcode, but what I read looks mainly ok to me.

Shouldn't we add a s.swift_version attribute in the podspec though?

Package.swift Outdated
import PackageDescription

let package = Package(
name: "Stencil",
products: [
.library(name: "Stencil", targets: ["Stencil"]),
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

OCD-me speaking: seems like there's inconsistent indentation here compared to the rest of the file (tab vs space probably?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed 👍

@yonaskolb
Copy link
Collaborator Author

Comments addressed and changelog entry added

CHANGELOG.md Outdated
@@ -8,7 +8,9 @@

### Breaking Changes

_None_
- Now requires Swift 4.1.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now requires Swift 4.1 or newer*

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@djbe
Copy link
Contributor

djbe commented Sep 2, 2018

Would you mind adding the Swift 4 Lexer.swift changes to this PR?

It seems we're going to need to do some more testing, to compare #225 and #226, maybe even add some sort of test to keep track of the performance (so it doesn't tank again), and it's easier if that PR doesn't also have changes related to the Swift migration.

@yonaskolb
Copy link
Collaborator Author

I'd rather leave Lexer.swift alone. To fix the warnings means redoing the substring accessing anyway. I'd rather not cause conflicts in the other performance PRs.

@@ -23,6 +23,7 @@
"osx": "10.9",
"tvos": "9.0"
},
"swift_version": "4.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're adding this property (and we have to), we should also add cocoapods_version. I think the swift_version property is available since CocoaPods 1.4.0.

@djbe
Copy link
Contributor

djbe commented Sep 3, 2018

Causing conflicts shouldn't be an issue TBH, the PRs will need a rebase anyway. I just thought that a minimal changeset for the old and new performance PRs might help us test why there's a difference.

@yonaskolb
Copy link
Collaborator Author

I just timed this with my tests. It looks like the last commit took it from 0.48 to 1.07...

@djbe
Copy link
Contributor

djbe commented Sep 10, 2018

Could it be the String( initializers? We may want to try to keep using SubStrings, as it avoids re-allocation/copy operations.

@yonaskolb
Copy link
Collaborator Author

I tried to keep things as substrings, but things like hasPrefix weren't available on linux

@djbe
Copy link
Contributor

djbe commented Sep 10, 2018

And if we define our own functions specifically for Linux?

Note: We should move this discussion to #226.

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