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

added code to remove default arguments from overridden function definition #89

Merged
merged 3 commits into from
Feb 7, 2019

Conversation

torlangballe
Copy link
Collaborator

No description provided.

Copy link
Owner

@angelolloqui angelolloqui left a comment

Choose a reason for hiding this comment

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

Approved and changed with some additional changes (see comments)

@@ -0,0 +1,5 @@
class Test {
override func dostuff(x:Int = 5) {
}
Copy link
Owner

Choose a reason for hiding this comment

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

great! this is the kind of tests useful. I am going to add a second method with default arguments not overriden to check they are not removed in that case

@@ -0,0 +1,5 @@
class Test {
override func dostuff(x:Int = 5) {
Copy link
Owner

Choose a reason for hiding this comment

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

Very minor, but it is good practice to try to keep formatting as close as possibe to the standard accepted Swift. In this case, the space between x and Int. Not a reason to reject the PR, but just a comment (I changed it myself)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes. I started using swift without spaces between label and type in arguments, but switched a while back, but still forget sometimes...

@@ -1078,3 +1084,32 @@ public struct InvertedCondition: ASTTokenizable {
public let condition: Condition
}

// function used to remove default arguments from override functions, since kotlin doesn't have them
private func removeDefaultArgsFromParameters(tokens:[Token]) -> [Token] {
var newTokens = [Token]()
Copy link
Owner

Choose a reason for hiding this comment

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

I am moving this function to a private method inside the main class. There is no real benefit of it since we are not inheriting for now, but just for consistency with the rest.

let bodyTokens = declaration.body.map(tokenize) ?? []

return [
if modifierTokens.contains(where:{$0.value == "override" }) {
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of checking the modifiers directly, I am going to move it to an extension like the isStatic because it is a handy check to be used by others in future (and safer to check the type directly rather than the text value)

@angelolloqui
Copy link
Owner

Merged and created Issue with the sample code for better tracking (#90)

@torlangballe
Copy link
Collaborator Author

Great. I'll delete my branch and start a new pull request on a new branch, does that sounds right?

PS: Next down the file was a call to something that tranformed function definitions to make operator functions in kotlin by prefixing them with operator_: in swift.
Not sure this is general enough for your repo, or if some kind of hook to transform function-definitions chould be added instead?

class func TransformKotlinFunctionDeclarations(_ tokens:[Token]) -> [Token] {
    // https://kotlinlang.org/docs/reference/operator-overloading.html
    var newTokens = tokens.filter { $0.kind != .keyword || $0.value != "mutating" }
    if let i = findToken(kind:.keyword, val:"fun", tokens:newTokens) {
        let j = i + 2
        let t = newTokens[j]
        if t.kind == .identifier {
            var name = ""
            // this allowes special operator_ prefixed functions to become operators in kotlin
            if stringHasPrefix(t.value, prefix:"operator_", rest:&name) {
                newTokens[j] = changedValueToken(t, name)
                if let origin = t.origin, let node = t.node {
                    newTokens.insert(origin.newToken(.space, " ", node), at:i)
                    let op = origin.newToken(.keyword, "operator", node)
                    newTokens.insert(op, at:i)

                }
            }
        }
    }
    return newTokens
}

@angelolloqui
Copy link
Owner

angelolloqui commented Feb 8, 2019

Great. I'll delete my branch and start a new pull request on a new branch, does that sounds right?

Yes, that should be the way to go, each feature in a new branch, always starting from the most recent commit on develop. That way we can merge them in isolation and keep conversations short.

@angelolloqui
Copy link
Owner

Regarding your operation question, do you have a concrete example? (Swift version and what is expected/obtained in Kotlin)

@torlangballe
Copy link
Collaborator Author

torlangballe commented Feb 8, 2019 via email

@angelolloqui
Copy link
Owner

angelolloqui commented Feb 8, 2019

I am not sure I understand why you have that operator_plus function. If you have this swift code:

func +(me:ZRect, a:ZRect) -> ZRect { return ZRect(min: me.pos + a.pos, max: me.max + a.Max) }

then this could be translated to Kotlin like:

operator fun plus(a: ZRect) : ZRect = ZRect(min = pos + a.pos, max = max + a.max)

and in both cases it could be used normally as:
rectA + rectB

Why do you need the artificial operator_plus? is it because one is a function and the other a method?

@angelolloqui
Copy link
Owner

BTW, there is a related issue #43, maybe we should move the conversation there

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.

2 participants