Skip to content

[ES7] Exponentiation#4914

Merged
yuit merged 42 commits into
masterfrom
exponentiation
Oct 12, 2015
Merged

[ES7] Exponentiation#4914
yuit merged 42 commits into
masterfrom
exponentiation

Conversation

@yuit

@yuit yuit commented Sep 21, 2015

Copy link
Copy Markdown
Contributor

Issues #4812

Comment thread src/compiler/parser.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can just keep this if (newPrecedence <= precedence)

Alternatively you could make an inner if:

// Check the precedence to see if we can "take" the operator.
// When the precedence is too high, we'll need to build up a right-heavy 
// expression by calling ourselves recursively below.
// Otherwise, we'll return the current node to our caller so that they can
// build a left-heavy expression using the current node as a right operand.
if (newPrecedence <= precedence) {
    // For left-associative operators, a caller can only accumulate left-heavy expressions
    // like this if the new precedence is less than or equal to than the current one.
    // However, for right-associative operators (i.e. the ** operator), the new operator's
    // precedence needs to be strictly less, since something like  a ** b ** c needs to be
    // parsed as the right-heavy expression a ** (b ** c).
    //
    // Here we ensure that only if the new operator is right-associative do we
    // check for strict equality on precedence.
    if (token !== SyntaxKind.AsteriskAsteriskToken || newPrecedence < precedence) {
        break;
    }
}

Comments optional, though I think they'd be useful 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think the above check is the same as

if(newPrecenden <= precendence) {
    break;
}

you want to also break if the operator has lower precedence not just equal.

a * b - c

will be parsed incorrectly as a * (b-c)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, as we discusssed offline, this should be

token !== SyntaxKind.AsteriskAsteriskToken || newPrecedence < precedence

Amended the above

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's just do

const canTakeNextOperator = token === SyntaxKind.AsteriskAsteriskToken ? 
    newPrecedence < precedence :
    newPrecedence <= precedence;

Thanks @weswigham 😄

@DanielRosenwasser

Copy link
Copy Markdown
Member

Can you test the emit for the following?

var a, b, c;
new a ** b ** c;
new a ** new b ** c;
new (a ** b ** c);

@DanielRosenwasser

Copy link
Copy Markdown
Member

Also, can you add a test for the following?

(<number><any>(1,2)) ** (<any>3,4)

@DanielRosenwasser

Copy link
Copy Markdown
Member

Also maybe an ES5/6 test with the following:

// Object literal binding assignment
// Causes an early error, but it is useful to see the emit.
({ toExponential } **= 1 ** 2);

@yuit

yuit commented Sep 24, 2015

Copy link
Copy Markdown
Contributor Author

what are you trying to test with this

var a, b, c;
new a ** b ** c;
new a ** new b ** c;
new (a ** b ** c);

@jeffreymorlan

Copy link
Copy Markdown
Contributor

Pre-emptive bug report: downlevel emit for **= duplicates expressions with side effects, e.g. a[i++] **= 2; becomes a[i++] = Math.pow(a[i++], 2). In general, an LHS of the form x.prop or x[y] may require x and/or y to be saved to a temp variable, if they are not simple variables/constants already.

There should also be an error if the global Math is masked by a local variable of the same name.

@yuit

yuit commented Sep 24, 2015

Copy link
Copy Markdown
Contributor Author

@jeffreymorlan good catch. Thanks! 💃

@yuit

yuit commented Sep 25, 2015

Copy link
Copy Markdown
Contributor Author

thanks @rbuckton for updating me about yet another discussion regarding the precedence of the operator (here is the link http://jsbin.com/bihilaveda/1/edit?output). So there may be a change on the precedence. Full discussion (https://esdiscuss.org/topic/exponentiation-operator-precedence)

Comment thread src/compiler/parser.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Move the comma to the preceding line, add a comma after "below"

Comment thread src/compiler/emitter.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Compound exponentiation

Comment thread src/compiler/commandLineParser.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i would remove this.

@yuit

yuit commented Oct 12, 2015

Copy link
Copy Markdown
Contributor Author

@mhegazy and @DanielRosenwasser let me know if you have anymore comments

Comment thread src/compiler/emitter.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldEmitCommaBeforeAssignment

yuit added a commit that referenced this pull request Oct 12, 2015
@yuit yuit merged commit 77eaf04 into master Oct 12, 2015
@yuit yuit deleted the exponentiation branch October 12, 2015 23:37
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants