Skip to content

Reuse getBinaryOperatorPrecedence#22844

Merged
3 commits merged into
masterfrom
getBinaryOperatorPrecedence
Mar 27, 2018
Merged

Reuse getBinaryOperatorPrecedence#22844
3 commits merged into
masterfrom
getBinaryOperatorPrecedence

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Mar 23, 2018

There are two functions that do basically the same thing.
I did notice that they return different results for **; had to preserve the old behavior to prevent a baseline from changing, but it may be better for the baseline to change if you write (void --temp) ** 3;; may be worth it to just change the baseline though, which seems harmless.

@ghost ghost requested a review from rbuckton March 23, 2018 21:54
@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 23, 2018

@rbuckton Also, I tried adding an assertion and the ExclamationToken and TildeToken cases seem to be unused -- those aren't binary expressions.

Comment thread src/compiler/utilities.ts
}
}

/* @internal */
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'd rather we keep to a consistent series of values for operator precedence, so we don't in the future accidentally try to compare the result from getBinaryOperatorPrecedence to getOperatorPrecedence. As far as the parser is concerned, it only cares whether the new precedence is higher than the old one and is greater than 0.

Comment thread src/compiler/utilities.ts Outdated

case SyntaxKind.AsteriskAsteriskToken:
// This would be 15 if we used the below line instead, which changes emit for the test `emitExponentiationOperator4`.
return 14;
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.

Its probably ok to bump this up to 15 since that reflects the current spec, though we should probably also bump the precedence of anything 15 or above up by one.

@rbuckton
Copy link
Copy Markdown
Contributor

@Andy-MS Yeah, ExclamationToken and TildeToken can be removed since they're actually PrefixUnaryExpression and are handled by that case instead.

Comment thread src/compiler/utilities.ts

/* @internal */
export function getBinaryOperatorPrecedence(kind: SyntaxKind): number {
switch (kind) {
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.

minor nit: It's a bit odd to read getOperatorPrecedence in descending order then see getBinaryOperatorPrecedence in ascending order. We might want to change the order of one or the other to be consistent.

@ghost ghost merged commit 07a890d into master Mar 27, 2018
@ghost ghost deleted the getBinaryOperatorPrecedence branch March 27, 2018 23:38
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
This pull request was closed.
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.

1 participant