Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes parsing of signed numeric literals #1484

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Fixes parsing of signed numeric literals #1484

merged 2 commits into from
Jul 16, 2024

Conversation

RCHowell
Copy link
Member

@RCHowell RCHowell commented Jun 6, 2024

Relevant Issues

#1482

Description

Applies +/- signs to numeric literals.

Other Information

  • Updated Unreleased Section in CHANGELOG: [YES/NO]
    No

  • Any backward-incompatible changes? [YES/NO]
    No

  • Any new external dependencies? [YES/NO]
    No

  • Do your changes comply with the Contributing Guidelines
    and Code Style Guidelines? [YES/NO]
    Yes

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@RCHowell RCHowell requested a review from alancai98 June 6, 2024 03:35
@RCHowell RCHowell requested review from yliuuuu and removed request for alancai98 June 18, 2024 18:49
Copy link
Contributor

@yliuuuu yliuuuu left a comment

Choose a reason for hiding this comment

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

Quote from the issue #1482

Given a numeric literal like -1, this becomes an AST like unary_minus(literal(1)) which has strange downstream implications. The desired behavior is to have the sign as part of the literal like literal(-1).

Could you please elaborate why you think unary_minus(literal(1)) is a bad idea?

I don't know if this is worth to attempt to lower some nodes during the parsing stage.

I see that you listed pretty printer as an example, but I feel like outputting -(1) is fine and if we want to change this, it might be better to modify the pretty printer instead of the parsing logic.

// If argument is not a literal, then return the op.
val arg = visit(ctx.rhs) as Expr
return when (arg) {
is Expr.Lit -> arg.negate(op)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this potentially can cause overflow:

-(-2147483648) => 2147483648 which can not be represented by 32 bit integer. 

Copy link

Conformance comparison report

Base (ac23bbc) 6ed8ff4 +/-
% Passing 92.64% 92.64% 0.00%
✅ Passing 5423 5423 0
❌ Failing 431 431 0
🔶 Ignored 0 0 0
Total Tests 5854 5854 0

Number passing in both: 5423

Number failing in both: 431

Number passing in Base (ac23bbc) but now fail: 0

Number failing in Base (ac23bbc) but now pass: 0

@RCHowell RCHowell requested a review from yliuuuu July 15, 2024 16:18
@RCHowell RCHowell merged commit d660231 into main Jul 16, 2024
10 checks passed
@RCHowell RCHowell deleted the number-signs branch July 16, 2024 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants