Skip to content

rustc: Tweak Literal APIs of proc_macro #48894

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

Closed

Conversation

alexcrichton
Copy link
Member

This commit tweaks the public api of the proc_macro::Literal structure to
solve #48889, an issue about negative integers and their representation in
proc_macro tokens. Currently the compiler assumes that all integer tokens are
positive integers rather than embedding a negative sign. The negative sign is
a separate token from the integer itself.

Currently there's a function like:

impl Literal {
    pub fn i32(i: i32) -> Literal;
}

but unfortunately this doesn't work for negative integers. When called with
negative integers weird errors end up getting thrown later during the parsing
process.

This function tweaks the definitions to instead use:

impl Literal {
    pub fn i32(i: u32) -> Literal;
}

This construction makes it more clear that negative arguments are not supported.
This additionally allows literals like -128i8 where 128, the positive
integer part of this literal, isn't representable in i8. By taking an unsigned
number in each constructor we can allow constructing the minimum literal for
each signed type as well.

The final tweak of this commit is to panic in Literal::{f32, f64} if the input
number is negative. This is similar to how infinite/nan literals are handled
today (they panic) and constructing a literal should be possible by negating a
float and then separately passing in a literal.

Closes #48889

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2018
@alexcrichton
Copy link
Member Author

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned nikomatsakis Mar 9, 2018
@@ -499,6 +512,9 @@ impl Literal {
if !n.is_finite() {
panic!("Invalid float literal {}", n);
}
if n < 0.0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like negative zeroes still pass the check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah oops, just found is_sign_negative which is what I was looking for earlier.

This commit tweaks the public api of the `proc_macro::Literal` structure to
solve rust-lang#48889, an issue about negative integers and their representation in
`proc_macro` tokens. Currently the compiler assumes that all integer tokens are
*positive* integers rather than embedding a negative sign. The negative sign is
a separate token from the integer itself.

Currently there's a function like:

    impl Literal {
        pub fn i32(i: i32) -> Literal;
    }

but unfortunately this doesn't work for negative integers. When called with
negative integers weird errors end up getting thrown later during the parsing
process.

This function tweaks the definitions to instead use:

    impl Literal {
        pub fn i32(i: u32) -> Literal;
    }

This construction makes it more clear that negative arguments are not supported.
This additionally allows literals like `-128i8` where `128`, the positive
integer part of this literal, isn't representable in `i8`. By taking an unsigned
number in each constructor we can allow constructing the minimum literal for
each signed type as well.

The final tweak of this commit is to panic in `Literal::{f32, f64}` if the input
number is negative. This is similar to how infinite/nan literals are handled
today (they panic) and constructing a literal should be possible by negating a
float and then separately passing in a literal.

Closes rust-lang#48889
@alexcrichton alexcrichton force-pushed the fix-literal-too-large branch from 713f86e to a6b957c Compare March 9, 2018 22:17
@nrc
Copy link
Member

nrc commented Mar 11, 2018

I'd prefer to address this by making the parser more permissive and accept negative literals - that doesn't feel like too much work and makes it easier to handle negative literals for proc macros.

@alexcrichton
Copy link
Member Author

Ok sounds like this isn't the direction the discussion on #48889 is headed, so closing

@alexcrichton alexcrichton deleted the fix-literal-too-large branch March 12, 2018 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants