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

revert Improve effect analysis of bitshifts #47567 #47830

Merged
merged 7 commits into from
Dec 13, 2022
Merged

revert Improve effect analysis of bitshifts #47567 #47830

merged 7 commits into from
Dec 13, 2022

Conversation

oscardssmith
Copy link
Member

This broke the BitIntegers package. It would be good to fix the original issue from the compiler side.

@giordano
Copy link
Contributor

giordano commented Dec 8, 2022

It would be good to fix the original issue from the compiler side.

Open an issue to keep track of that?

@oscardssmith oscardssmith mentioned this pull request Dec 8, 2022
Open
@KristofferC
Copy link
Sponsor Member

This broke the BitIntegers package.

Okay but why? What is the bug in this PR and should a corresponding test be added so that it doesn't regress again?

KristofferC
KristofferC reviewed Dec 8, 2022
View reviewed changes
test/int.jl Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Member Author

@KristofferC it wasn't a bug, just that the way it widened applicable types caused ambiguity errors for packages that tried to extend it.

@aviatesk
Copy link
Sponsor Member

aviatesk commented Dec 9, 2022

just that the way it widened applicable types caused ambiguity errors for packages that tried to extend it.

Could you add a simplified test case, i.e. the operation that caused the issue now doesn't widen them?

@brenhinkeller brenhinkeller added the kind:bugfix This change fixes an existing bug label Dec 11, 2022
@aviatesk
Copy link
Sponsor Member

@oscardssmith bump?

@oscardssmith
Copy link
Member Author

sorry, I did a bad job explaining what is wrong here. the PR didn't change any values, but meant that new types wouldn't work. I'm not sure there is a great way to test for this.

aviatesk
aviatesk reviewed Dec 13, 2022
View reviewed changes
test/int.jl Outdated Show resolved Hide resolved
@aviatesk aviatesk merged commit 9fa08cc into JuliaLang:master Dec 13, 2022
@oscardssmith oscardssmith deleted the revert-47567 branch December 13, 2022 03:45
@vtjnash vtjnash mentioned this pull request Dec 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KristofferC KristofferC KristofferC left review comments

@aviatesk aviatesk aviatesk left review comments

@quinnj quinnj quinnj approved these changes

Assignees
No one assigned
Labels
kind:bugfix This change fixes an existing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@oscardssmith @giordano @KristofferC @aviatesk @quinnj @brenhinkeller