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

Closes #3281 #3285

Merged
merged 2 commits into from
Jun 4, 2013
Merged

Closes #3281 #3285

merged 2 commits into from
Jun 4, 2013

Conversation

amitmurthy
Copy link
Contributor

Closes #3281

Sorry about the multiple open pull requests, I am yet to get the complete hang of using git in this project here.

@StefanKarpinski
Copy link
Member

Looks good. This also includes several isprime and corelib changes though – let's review those separately.

@amitmurthy
Copy link
Contributor Author

The isprime() changes were adding additional function signatures for 64-bit integers on 32-bit machines. Without them a memory corruption was occurring (I suspect due to the bit shift operations) in isprime(n::Integer).

The corelib changes are in test/corelib.jl since on a 32-bit machine, the wrong range is being returned for

julia> (-4*int64(maxintfloat())):5
-36028797018963968:-36028797018963963

Changed to

julia> (-4*int64(maxintfloat(Float32))):5
-67108864:5

it works as expected and the contains test goes through.

@StefanKarpinski
Copy link
Member

If you get rid of that commit, I'm happy to merge this. You can do git rebase -i origin/master and then edit the file it presents you to get rid of this commit. Then just do a force push to this pull request branch.

@StefanKarpinski
Copy link
Member

To clarify, I'm not certain either of those things are quite what we want, but the other stuff is clearly ok.

@pao
Copy link
Member

pao commented Jun 4, 2013

You can do git rebase -i origin/master and then edit the file it presents you to get rid of this commit.

That will drop that commit object on the floor, which is probably not what you want either.

$ git checkout -b fix32bit origin/master
$ git cherry-pick b0c8201
$ git checkout master
$ git reset --hard HEAD^ # simpler way to drop the last commit than a rebase
$ git push -f amitmurthy HEAD

Then you can checkout the fix32bit branch, push it, and start another pull request.

I'll reiterate my standard advice to never do work on the master branch directly, in part because it makes these instructions more complicated to write (keeping track of whose master is whose).

@amitmurthy
Copy link
Contributor Author

I did the following and it seems to have worked:

git rebase -i HEAD~3 and removed the last commit
git push -f

@amitmurthy
Copy link
Contributor Author

@pao , in this case the last commit was intended to be dropped. The fix requires an entirely different solution.

StefanKarpinski added a commit that referenced this pull request Jun 4, 2013
@StefanKarpinski StefanKarpinski merged commit d7a2ffa into JuliaLang:master Jun 4, 2013
@pao
Copy link
Member

pao commented Jun 4, 2013

Ahh, I misunderstood and took it as "these changes should be considered separately" instead of "let's forget this ever happened". At any rate, the git reset --hard HEAD^ trick is still a valid alternative to rebasing when you're getting rid of the most recent commits.

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.

3 participants