Wiki

Ticket #114 (closed defect: fixed)

Opened 11 years ago

Last modified 7 years ago

Missing implementation of power/PowerTo/PowerToEquals Binary Op

Reported by: hopscc Owned by: Chuck
Priority: major Milestone:
Component: Cobra Compiler Version: 0.8.0
Keywords: Cc:

Description

If write a program that uses the power binary op (STARSTAR '**')
as in

return Math.sqrt(Math.abs(x)) + (5.0f * x ** 3.0f)

get an assert error in BinaryOP.cobra

Unhandled Exception: Cobra.Lang.AssertException:
sourceSite = C:\home\hops\src\cobra\wkspace\Source\BinaryOpExpr.cobra:455 in BinaryMathExpr.init for object BinaryMathExpr-sh(5373, didBindInh=false, didBindInt=false, didBindImp=false, token=Token(STARSTAR, '**', '**', ln 5, col 26, TPK.cobra), type=nil, 5373)
info       = nil
this       = BinaryMathExpr-de(5373, didBindInh=false, didBindInt=false, didBindImp=false, token=Token(STARSTAR, '**', '**', ln 5, col 26, TPK.cobra), type=nil, op=STARSTAR, left=IdentifierExpr-sh(5371, didBindInh=false, didBindInt=false, didBindImp=false, token=Token(ID, 'x', 'x', ln 5, col 24, TPK.cobra), name=x, type=nil, 5371), right=IntegerLit-sh(5372, didBindInh=false, didBindInt=false, didBindImp=false, token=Token(INTEGER_LIT, '3', 3, ln 5, col 29, TPK.cobra), type=nil, 5372), 5373)
    (op in ['PLUS', 'MINUS', 'STAR', 'SLASH', 'SLASHSLASH', 'PERCENT']) = false
        op = 'STARSTAR'
        ['PLUS', 'MINUS', 'STAR', 'SLASH', 'SLASHSLASH', 'PERCENT'] = List<of String>['PLUS', 'MINUS', 'STAR', 'SLASH', 'SLASHSLASH', 'PERCENT']

Due missing 'STARSTAR' entry in assert list on that line.
If correct that get c# error re missing method CobraImp.PowerTo

 error: "Cobra.Lang.CobraImp" does not contain a definition for "PowerTo" (C#)

At the very least an unimplemented operator should be trapped and emit an error message admitting its unimplemented
otherwise implement the powerTo/powerToEquals methods in Native.cs

Attachments

pow-expr.patch Download (2.8 KB) - added by hopscc 10 years ago.
pow-expr2.patch Download (4.7 KB) - added by hopscc 10 years ago.

Change History

Changed 10 years ago by hopscc

Changed 10 years ago by hopscc

  • status changed from new to assigned
  • owner set to Chuck

min implementation of '**' operator, rel note and test.

Changed 10 years ago by Chuck

  • owner Chuck deleted

Applied, but I'm leaving the ticket open for the completion of this feature:

This patch uses Math.Pow, but I think we'll need to return to a CobraLangInternal.CobraImp.PowerToEquals so that an efficient implementation for ints can be provided.

Also, missing support for **=

Some feedback:

Don't use assert x.getType is Double because that will not work with other back-ends. Use something portable like:

assert z inherits float  # .warning.  The expression is always of type "float"

Rather than checking assert x in [100, 1000, 10000, 100000] use something more precise like:

    for i, answer in [[2, 100], [3, 1_000], [4, 10_000], [5, 100_000]]
        x = 10 ** i
        assert x inherits int  # .warning. The expression is always of type "int"
        assert x == answer

Changed 10 years ago by hopscc

Changed 10 years ago by hopscc

  • owner set to Chuck

Change to use CobraLangInternal.CobraImp method PowerTo which is wrapper around Math.Pow for floats
and a simple iterative multiplication implementation for ints.
Slight change to handling in AugAssignMathExpr.SharpDefOperation to accommodate the method call rather than a C# operator.
Augmented test for **= and exercising int implementation.
Commented Timing test shows int implementation 25x faster than Math.Pow (:-)

Changed 10 years ago by Chuck

Sounds great, just couldn't get to this one tonight. Thanks.

Changed 10 years ago by Chuck

Hmm, Python says:

2 ** -5

0.03125

We might need to account for negative values on the right side.

Changed 10 years ago by hopscc

Well

2 ** -5.0f 

and

2.0f ** -5

gives the same answer as python (forcing the return type to float)

With an int,int -> int (return type) though what is a meaningful answer ?
0 or an Exception ( and note to explicitly specify a float input type) ??
Especially given the int,int -> int is a performance tweak ... (positive cases only)

Or do you want to return an int type for positive powers and a float type for negative ones?

Changed 10 years ago by hopscc

Enquiring minds (still) want to know.

Changed 7 years ago by Charles

Patch applied in changeset:2849

Changed 7 years ago by Charles

I don't think it's possible for Cobra to return an int type for positive powers and a float for negative because when the power term is a variable, you cannot know until run-time.

For division, we handle this with two operators: / and //... The first always returns a number (decimal or float64 or float32) and the second is always for ints. Two different operations.

(As a side note: i /= j where i is an int, compiles and runs, but should really be an error.)

We could have two operators for ** though it's not clear what they should be. Or we could simply make ** for two ints throw an exception if the power is < 0, thereby requiring the person to cast the base to a number and thereby change the type of the expression to number. **= would also have throw an error too. I think I like this last option best.

Changed 7 years ago by Charles

  • status changed from assigned to closed
  • resolution set to fixed

changeset:2859 changeset:2860

i ** j where both are ints and j is negative will now throw an InvalidOperationException.

** now works for decimals as well.

AFAIK we're done unless someone wants to enhance Native.cs' PowTo(decimal, decimal) implementation which will, in some cases, fall back to casting to C# double and calling Math.Pow.

Note: See TracTickets for help on using tickets.