Closed horasal closed 8 years ago
@zhaihj Big thanks for submitting a fix! We are currently testing and reviewing this. :+1:
Overall, we're quite happy with this PR. One thing we found is that rock accepts this without warning (gcc fails, however). We would request that since gcc fails, rock should produce an error in this case:
Base: abstract class {
init: func
increase: virtual func {
this value += 1
}
}
Derived: class extends Base{
init: func { super() }
// rock seems to ignore that increase() is returning void in the base class
// instead, this should cause rock to produce an error (not warning)
increase: override func -> Int {
this value + 1
}
}
@zhaihj almost there, however:
Base: abstract class {
value: Int
init: func { this value = 0 }
increaseByN: abstract func (n: Int) -> Int
increaseByThree: virtual func -> Int {
this value += 3
}
}
Derived: class extends Base {
init: func { super() }
increaseByN: override func (n: Float) -> Float {
this value += n
}
increaseByThree: override func -> Float {
this value += 3.0f
}
}
Here, rock ignores that the type is Int
in the base, and Float
in the derived class.
Hmm, this is because that current rock allow matching different types of numbers -- so it can allow you return a Int
in something like func->Float
. I don't want to touch the getScoreImpl function because it will breaks too many things. As the result, I make the definition-check follows the following rule:
rock is really relaxed with what you can override indeed.
It should get stricter at some point but I'm pretty sure a lot of things in the core Number type declarations would break :p
@shamanas
Rock even allow define default parameter before the normal ones :laughing:
Well, when it comes to numbers, this seems logical. We do want stuff like myFloat: Float = myInt
to work, so this seems like a good solution.
How about if you change Int
to Float
of an argument type? We should do the same thing then.
@zhaihj do you have additional commits coming? Otherwise, as far as I'm concerned, this is a done deal. Now it's up to @fredrikbryntesson to sign off on it.
@thomasfanell currently that's all!
@zhaihj It would be good if you could write a short explanation about how you reasoned when you created the solution as stated in the bounty repository. "Your pull request's description should contain a brief summary of what has been done, how it works and how the solution was found. " A simple writeup about how you solved the problem.
Update pull request description (I'm not good at writing :(
@zhaihj We accept it, send me a mail at fredrik.bryntesson@vidhance.com to collect the reward.
@zhaihj Very nicely done, and very quick responses, thank you. :)
Summary
This pull request allow rock check if there exists any incompatible definition between derived and base classes (covers). If there are any incompatible definitons, a warning is generated. The rules for checking is summarized as follows:
super
function call vs definitionsuper
should match the best matched definitoin.Implementation Details
super
call is unwrapped inmiddle/FunctionCall.ooc
. When resolving a function call, rock checks if its name equals tosuper
. Once a super call is found, rock will perform a search the best definition in themeta class
(which contains all the functions). The auto-fordwarding of arguments also happens here. A extra check on the number of arguments is added after the definition is got.magic-lang/rock
, every function derived from the base class should have theoverride
property. Override functions are guaranteed to only derived from abstract/virtual function byTypeDecl.ooc:checkOverrideFuncs
. Checking of compatiblity is also performed incheckOverrideFuncs
. One thing should be point out is that it is necessary to unwrapThis
before checking becausegetScore
can not matchThis
agaist its actural definition.