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.

Sign up for GitHub

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

Jump to bottom

Added lint for TryFrom for checked integer conversion. #4088

Merged
merged 9 commits into from
May 20, 2019
Merged

Added lint for TryFrom for checked integer conversion. #4088

merged 9 commits into from
May 20, 2019

Conversation

pJunger
Copy link
Contributor

@pJunger pJunger commented May 12, 2019 •
edited by oli-obk
Loading

works towards #3947

Added lint for try_from for checked integer conversion.
Should recognize simple & straight-forward checked integer conversions.

@pJunger pJunger changed the title Added lint for TryFrom for checked integer conversion rust-lang#3947. Added lint for TryFrom for checked integer conversion. May 12, 2019
@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 13, 2019
oli-obk
oli-obk suggested changes May 14, 2019
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Sorry about taking so long to review this!

I don't think this closes the issue fully, as it only gets rid of the handcrafted conversion check, but the actual conversion via as still stays around. This can be addressed in a follow up PR though.

tests/ui/checked_conversions.stderr Show resolved Hide resolved
@pJunger
Copy link
Contributor Author

pJunger commented May 14, 2019

No worries!
I agree that this is only a partial solution - though I guess it's the part that's more error prone (and easier to implement).
Should I remove the 'Closes' comment?

@oli-obk
Copy link
Contributor

oli-obk commented May 14, 2019

Should I remove the 'Closes' comment?

yea, we'll keep the issue open for the full fix

@oli-obk
Copy link
Contributor

oli-obk commented May 14, 2019

you have some CI failures. Most likely you need to rebase and use the latest master branch of rustc.

@oli-obk
Copy link
Contributor

oli-obk commented May 17, 2019

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented May 17, 2019

📌 Commit e85211c has been approved by oli-obk

@bors
Copy link
Contributor

bors commented May 17, 2019

⌛ Testing commit e85211c with merge e8043b5...

bors added a commit that referenced this pull request May 17, 2019
@bors
Auto merge of #4088 - pJunger:check1, r=oli-obk
e8043b5 
Added lint for TryFrom for checked integer conversion.

works towards #3947

Added lint for try_from for checked integer conversion.
Should recognize simple & straight-forward checked integer conversions.
@bors
Copy link
Contributor

bors commented May 17, 2019

💔 Test failed - status-appveyor

@bors
Copy link
Contributor

bors commented May 18, 2019

☔ The latest upstream changes (presumably #4110) made this pull request unmergeable. Please resolve the merge conflicts.

@pJunger
Copy link
Contributor Author

pJunger commented May 20, 2019

@oli-obk Could we try again? (I assume the last PR fixed the testsuite issue?)

@oli-obk
Copy link
Contributor

oli-obk commented May 20, 2019

@bors r+

sorry about the symbol churn.

@bors
Copy link
Contributor

bors commented May 20, 2019

📌 Commit 565feb0 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented May 20, 2019

⌛ Testing commit 565feb0 with merge fd56381...

bors added a commit that referenced this pull request May 20, 2019
@bors
Auto merge of #4088 - pJunger:check1, r=oli-obk
fd56381 
Added lint for TryFrom for checked integer conversion.

works towards #3947

Added lint for try_from for checked integer conversion.
Should recognize simple & straight-forward checked integer conversions.
@bors
Copy link
Contributor

bors commented May 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing fd56381 to master...

@bors bors merged commit 565feb0 into rust-lang:master May 20, 2019
@bors bors mentioned this pull request May 20, 2019
Merged
@pJunger pJunger deleted the check1 branch May 20, 2019 10:38
@pJunger
Copy link
Contributor Author

pJunger commented May 20, 2019

Sure! I'm glad it worked out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pJunger @oli-obk @bors @flip1995