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

wherein we suggest float for integer literals where a float was expected #53283

Merged

Conversation

zackmdavis
Copy link
Member

@sunjay pointed out that this is a nice thing that we could do.

Resolves #53280.

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2018
@sunjay
Copy link
Member

sunjay commented Aug 12, 2018

This is awesome! Thank you for doing this so fast! 😄

Will this code work if the integer literal has underscores in it? What about negative numbers? The full spec of integer literals is here: https://doc.rust-lang.org/reference/tokens.html#integer-literals

My personal opinion is that you don't need to support all forms of integers, but we should at least support literals in the format 1_000_000 since that is equivalent to 1000000. Negative integers are also a must.

Thanks again! This seems like a small thing, but I think it's a big quality of life improvement for people who are new to Rust! 😃

@GuillaumeGomez
Copy link
Member

@zackmdavis Please add tests for all cases that @sunjay gave. 😆

Sunjay Varma pointed out that this is a nice thing that we could do.

Resolves rust-lang#53280.
@zackmdavis zackmdavis force-pushed the and_the_case_of_the_flotation_device branch from 585793e to 58f660f Compare August 12, 2018 17:22
@zackmdavis
Copy link
Member Author

Updated and added UI test.

r? @GuillaumeGomez

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Love the change.

r=me once travis is happy.

@@ -250,6 +250,21 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
db.note("no two closures, even if identical, have the same type");
db.help("consider boxing your closure and/or using it as a trait object");
}
match (&values.found.sty, &values.expected.sty) { // Issue #53280
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice to provide a bit of context in the comments without the need to open a browser :)

As a rule, I add a one sentence long description of what the code is doing (or rather, why), with the issue number at the end for extra context.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that issue numbers are merely an extra reference for the curious software-archeologist and not a substitute for explanatory comments where explanatory comments are merited, but I feel like "use a float literal" is pretty self-explanatory? I do write explanatory comments where I think it necessary (recent examples: 1 2).

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 12, 2018

📌 Commit 58f660f has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2018
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 12, 2018
…ation_device, r=estebank

wherein we suggest float for integer literals where a float was expected

@sunjay pointed out that this is a nice thing that we could do.

Resolves rust-lang#53280.

r? @estebank
bors added a commit that referenced this pull request Aug 12, 2018
Rollup of 15 pull requests

Successful merges:

 - #52955 (Update compiler test documentation)
 - #53019 (Don't collect() when size_hint is useless)
 - #53025 (Consider changing assert! to debug_assert! when it calls visit_with)
 - #53059 (Remove explicit returns where unnecessary)
 - #53165 ( Add aarch64-unknown-netbsd target)
 - #53210 (Deny future duplication of rustc-ap-syntax)
 - #53223 (A few cleanups for rustc_data_structures)
 - #53230 ([nll] enable feature(nll) on various crates for bootstrap: part 4)
 - #53231 (Add let keyword doc)
 - #53240 (Add individual documentation for <integer>`.swap_bytes`/.`reverse_bits`)
 - #53253 (Remove unwanted console log)
 - #53264 (Show that Command can be reused and remodified)
 - #53267 (Fix styles)
 - #53273 (Add links to std::char::REPLACEMENT_CHARACTER from docs.)
 - #53283 (wherein we suggest float for integer literals where a float was expected)

Failed merges:

r? @ghost
@bors bors merged commit 58f660f into rust-lang:master Aug 13, 2018
@zackmdavis zackmdavis deleted the and_the_case_of_the_flotation_device branch August 13, 2018 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants