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

More promote_op fixes #17114

Closed
wants to merge 3 commits into from
Closed

Conversation

pabloferz
Copy link
Contributor

@pabloferz pabloferz commented Jun 25, 2016

@nalimilan

Note: This PR is against branch nl/promote_op instead of master

Previously triggered a stack overflow due to recursive definition.
Use common promote_op() based on one() for all Number types:
this fixes promote_op(+, ::Bool), which returned Int, and
promote_op(==, ::Complex, ::Complex), which returned Complex{Bool}.
Also fix a few corner cases which did not work or were type-instable.
Add systematic tests to catch this kind of bug.
@test @inferred([m m].*[m,m]) == [m2 m2; m2 m2]
convert{T<:Real,pow}(::Type{MeterUnits{T,pow}}, y::Real) = MeterUnits{T,pow}(convert(T,y))

@test m+[m,m] == [m+m,m+m]
Copy link
Member

@nalimilan nalimilan Jun 27, 2016

Choose a reason for hiding this comment

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

Why remove the @inferred calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a mistake while copying some of the code I tested locally. Is fixed now.

@nalimilan
Copy link
Member

nalimilan commented Jun 27, 2016

Thanks! Though @JeffBezanson is the one to ask...

Ref: #16995

@nalimilan nalimilan mentioned this pull request Jun 27, 2016
@nalimilan nalimilan force-pushed the nl/promote_op branch 2 times, most recently from 21bc568 to 9df4f1a Compare June 28, 2016 21:01
@nalimilan
Copy link
Member

I've picked your commit into #16995, let's continue there.

@nalimilan nalimilan closed this Jun 28, 2016
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