Skip to content

std.math.findRoot 1. can now accept delegate with different value and argument types 2. allow tolerance have default value #2266

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
Merged
merged 4 commits into from
Aug 14, 2014
Merged

std.math.findRoot 1. can now accept delegate with different value and argument types 2. allow tolerance have default value #2266

merged 4 commits into from
Aug 14, 2014

Conversation

9il
Copy link
Member

@9il 9il commented Jun 23, 2014

#1894 #1893 reopen

@9il
Copy link
Member Author

9il commented Jun 23, 2014

../dmd/src/dmd -I../druntime/import -w -m64 -O -release -unittest -c -ofgenerated/osx/release/64/unittest/std/numeric.o -deps=generated/osx/release/64/unittest/std/numeric.deps.tmp std/numeric.d
Internal error: backend/cg87.c 925

@9il
Copy link
Member Author

9il commented Jun 23, 2014

ICE 925 https://issues.dlang.org/show_bug.cgi?id=12057

@quickfur
Copy link
Member

The ICE appears to have been fixed. I tried to merge into master, and it compiles and runs unittests fine. Could you please rebase to push the autotester to retest this PR? Thanks!

@9il
Copy link
Member Author

9il commented Jul 17, 2014

@quickfur Thanks! I suppose PR rebased now. If it is not, please write me what I should do.

@9il
Update numeric.d
0747cc2 
clear comment
@mihails-strasuns
Copy link

Seems to still trigger the ICE

@9il
Copy link
Member Author

9il commented Jul 17, 2014

ICE 64 bit only

@quickfur
Copy link
Member

Gah, apparently the ICE is triggered only with -O; it worked fine when I tried to compile it with -main -unittest (without -O). Sounds like an optimizer bug.

quickfur
quickfur reviewed Jul 18, 2014
View reviewed changes
std/numeric.d
unittest
{
static assert(__traits(compiles, findRoot((float x)=>cast(real)x, float.init, float.init)));
static assert(__traits(compiles, findRoot!real((x)=>cast(double)x, real.init, real.init)));
Copy link
Member

Choose a reason for hiding this comment

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

Found the ICE: it's caused by this line. I'm going to run dustmite on it to find the minimal code that causes this.

@quickfur
Copy link
Member

Walter fixed the ICE. Hopefully the autotester will pass now...

@quickfur
Copy link
Member

Looks like the previous failures are passing now. Should we merge? @Dicebot

@mihails-strasuns
Copy link

Please update the PR / commit to briefly explain intention of the change. "findRoot fix" says nothing to the reader - for me at least.

@9il 9il closed this Jul 27, 2014
@9il 9il changed the title findRoot fix std.math.findRoot 1. can now accept delegate with different value and argument types 2. allow tolerance get default value Jul 27, 2014
@9il 9il changed the title std.math.findRoot 1. can now accept delegate with different value and argument types 2. allow tolerance get default value std.math.findRoot 1. can now accept delegate with different value and argument types 2. allow tolerance have default value Jul 27, 2014
@9il 9il reopened this Jul 27, 2014
@9il
Copy link
Member Author

9il commented Jul 27, 2014

@Dicebot, I have changed PR header. Is it what you suggest?

@9il 9il closed this Jul 27, 2014
@9il 9il reopened this Jul 27, 2014
@mihails-strasuns
Copy link

Pretty much. Extra points if you can squash commits and amend the resulting commit message to something similar :) If you don't know how to do it:

# squashing
git fetch upstream
git rebase -i upstream/master
# text editor will open
# mark all commits but the first one as "s" (squash) instead of default "p" (pick)
# save and exit

# amending
git commit --amend
# text editor will open
# change commit message : first line is commit message title, rest go to commit message body

git push -f origin patch-2

It is not necessary but appreciated.

Anyway: I don't know a thing about numeric stuff so need another LGTM from other reviewer before merging.

@9il
Copy link
Member Author

9il commented Jul 27, 2014

@Dicebot thank you! Next PR i will make properly. I created this one with github web-editor.

@quickfur
Copy link
Member

quickfur commented Aug 5, 2014

ping
This code is ready to merge, we just need it to be rebased. Please update. :-)

@quickfur
Copy link
Member

Auto-merge toggled on

quickfur pushed a commit that referenced this pull request Aug 14, 2014
Merge pull request #2266 from 9il/patch-2
05bdee0 
std.math.findRoot  1. can now accept delegate with different value and argument types 2. allow tolerance have default value
@quickfur quickfur merged commit 05bdee0 into dlang:master Aug 14, 2014
@9il 9il mentioned this pull request Apr 28, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@9il @quickfur @mihails-strasuns