alexschneider / teascript

5 stars 2 forks source link

Jsc exponent op #51

Closed juansc closed 9 years ago

juansc commented 9 years ago

Hey you guys! I added exponents and removed the '--' and '++' from our syntax, parser, and scanner.

Quick!

# Exponents are right associative per mathematical convention
2 ** 3 ** 4 # This is 2 ** (3 ** 4)

# A unary negation binds tighter on the right....
2 ** -3 # This is 1/8

# But binds less tightly on the left
-2 ** 4 # This is -(2 ** 4) = -16, not (-2**4) = 16

@rachelriv @alexschneider @whusted Please review and merge.

rtoal commented 9 years ago

Good work on determining the desired precedence and associativity, Juan.

However your grammar

 Exp5    ::=  prefixop? Exp6
 Exp6    ::=  (Exp7 '**')* Exp7
 Exp7    ::=  Exp8 (('.' Exp8) | ('[' Exp8 ']') | ('(' arglist ')'))*
 Exp8    ::=  boolLit | intLit | floatLit | id | '(' Exp ')' | StringLit | TupLit | SetLit | MapLit | ListLit | Range | Slice | nonelit

does not appear to me to pick up 2 ** -3 because after the ** you can only have an Exp7, which cannot begin with a prefixop. Or can it?

whusted commented 9 years ago

@rtoal is right, we have held off on exponents for now. What does everyone else think? I'm fine with having them in, we just have to make sure to change the macro to match it.

rtoal commented 9 years ago

I have written a prototype grammar that handles exponentiation and unary negation properly on the Keck Lab board. I've explained it to Zane and Alex. Alex gets it. Not sure if Zane does. Before it gets erased, go take a peek. If it's gone, try to come up with it yourself. :)

juansc commented 9 years ago

@rtoal Can you or @alexschneider take a picture?

@rtoal I have implemented the following change right now. Does this implementation make sense?

2 ** -4 ** -3 # This is 2 ** ( -( 4 ** -3 ) )

In this way the rightmost unary negation binds to the three, but the unary negation on the four applies to what's to the right of it.

juansc commented 9 years ago

I don't mind leaving out exponents, but I don't think the changes to our macrosyntax and our scanner and parser are so great we should leave them out.

juansc commented 9 years ago

Here is a proposed modified macrosyntax

 Exp5    ::=  prefixop? Exp6
 Exp6    ::=  Exp7 | ( Exp7 '**' (Exp5 '**')* Exp5)
 Exp7    ::=  Exp8 (('.' Exp8) | ('[' Exp8 ']') | ('(' arglist ')'))*

I'm not sure this conserves the right-associativity of the exponents though.

alexschneider commented 9 years ago

@juansc do you mind merging master onto your branch and resolving any conflicts?

Also, I think that the macrosyntax would be better and clearer this way:

Exp6    ::=  Exp7 ('**' Exp5)?
juansc commented 9 years ago

@alexschneider I'm not sure if that macrosyntax implies that exponentiation is left-associative, rather than right-associative.

alexschneider commented 9 years ago

@juansc: I'm not confident that the macrosyntax implies anything about associtivity - I thought that's all done within the parser itself. Check the pic for what @rtoal mentioned

I also changed it slightly since you last saw it, so check the updated comment.

rachelriv commented 9 years ago

We can check out Python's grammar for reference.

In Python, the power operator binds more tightly than unary operators on its left; it binds less tightly than unary operators on its right.

power ::=  primary ["**" u_expr]
rachelriv commented 9 years ago

@juansc It looks like some coffeelint things are failing.

You can test it by running coffeelint . from the root of this directory to see why it is complaining.

juansc commented 9 years ago

@rachelriv Thanks I will do it right now and let you know if I have questions.

alexschneider commented 9 years ago

@rachelriv @juansc, if you use SublimeLinter it will tell you about these issues as you're writing the code.

juansc commented 9 years ago

The only thing that fails coffeelint now is the coffee-coverage. I don't touch the stuff. Everything else should work.

alexschneider commented 9 years ago

Also, I would feel more comfortable if there was a separate test for the ** operator, but if @rachelriv or @whusted is comfortable with merging this as is, then LGTM.

whusted commented 9 years ago

@juansc I'd like to see a quick exponent test as a sanity check before merging

juansc commented 9 years ago

There are 5 tests. In the seventh teascript program I had added

3 ** 4 + 5
-2 ** 4 + 5
3 ** 4 ** 5
2 ** 1 ** -3 + 1
2 ** -3 ** -4

these get parsed into

(+ (** 3 4) 5)
(+ (- (** 2 4)) 5)
(** 3 (** 4 5))
(+ (** 2 (** 1 (- 3))) 1)
(** 2 (- (** 3 (- 4))))
alexschneider commented 9 years ago

@juansc: I think what Willy meant is that it should be in a separate teascript program with a separate test (separate describe, it, etc)

juansc commented 9 years ago

Can I just create program14.tea and copy over those tests?

alexschneider commented 9 years ago

I've already created program13.tea in one of my merges, I'd prefer if you created program14.tea.

juansc commented 9 years ago

That's what I said program14.tea

whusted commented 9 years ago

That's what I meant, thanks for clarifying, Alex. @juansc it's more to match the format of how we're testing and keep everything consistent

alexschneider commented 9 years ago

@juansc I've got photographic evidence!

juansc commented 9 years ago

Cool I just added it.

juansc commented 9 years ago

@alexschneider It's obviously been doctored.

rachelriv commented 9 years ago

@juansc great test cases! Thanks for thinking through what our precedence/associativity should be for the ** operator. Once we update the macrosyntax, this looks good to me.

rachelriv commented 9 years ago

Make sure that you escape single quotes within the AST now.

juansc commented 9 years ago

All my tests failed when I merged with master. What's going on?

alexschneider commented 9 years ago

@juansc, #53 changed how errors are handled - rather than returning a map of the error and result, it just returns the result and throws an exception if we have an error.

juansc commented 9 years ago

@whusted @rachelriv @alexschneider Does this look fine to merge?

rachelriv commented 9 years ago

@juansc Looks good to me. :tea: :smile: :+1:

whusted commented 9 years ago

Me too