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

Do zero checking to avoid unnecessary work in addToExpression #536

Merged
merged 1 commit into from
Aug 15, 2015

Conversation

joehuchette
Copy link
Contributor

ref #535

@IainNZ
Copy link
Collaborator

IainNZ commented Aug 15, 2015

Nice. Just needs 0.3

@joehuchette
Copy link
Contributor Author

@IainNZ is this the same (terrifying) Travis error you were seeing?

@IainNZ
Copy link
Collaborator

IainNZ commented Aug 15, 2015

Sure is

@IainNZ
Copy link
Collaborator

IainNZ commented Aug 15, 2015

I kinda love that AppVeyor is finally the one passing and the others are failing

@@ -39,15 +41,19 @@ function addToExpression(aff::GenericAffExpr,c::Number,x::Number)
end

function addToExpression{C,V}(aff::GenericAffExpr{C,V}, c::Number, x::V)
Copy link
Member

Choose a reason for hiding this comment

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

Is it me or is this method identical to the one above on line 31?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, they're the same

joehuchette added a commit that referenced this pull request Aug 15, 2015
Do zero checking to avoid unnecessary work in addToExpression
@joehuchette joehuchette merged commit c87777e into master Aug 15, 2015
@joehuchette
Copy link
Contributor Author

The Travis failure looks like a recompilation issue with DataStructures; I can reproduce locally.

@joehuchette
Copy link
Contributor Author

Actually I think DataStructures was just in a broken state, I officially have no idea what's going on now.

@mlubin
Copy link
Member

mlubin commented Aug 15, 2015

Report upstream? I can reproduce with using JuMP.

@IainNZ IainNZ mentioned this pull request Aug 16, 2015
@IainNZ IainNZ deleted the checkzero branch September 5, 2015 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants