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

fix base(b, big(0), 0) == "0" #22133

Merged
merged 1 commit into from
Jul 11, 2017
Merged

fix base(b, big(0), 0) == "0" #22133

merged 1 commit into from
Jul 11, 2017

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented May 30, 2017 •
edited
Loading

When pad==0, base should give "" on input big(0).
I merged the two versions base(b, ::BigInt) and base(b, ::BigInt, pad). Without pad, it gets about 10% slower (so I could re-add the old version if requested), but the version with a pad is vastly faster (it gets faster when pad gets bigger).
Also I'm not sure: the buffer sent to libgmp must contain space for a final '\0', do I need to handle that manually or Sting allocates automatically 1 byte for it?

@rfourquet rfourquet added domain:bignums BigInt and BigFloat kind:bugfix This change fixes an existing bug labels May 30, 2017
@rfourquet
Copy link
Member Author

Bump (CI failure is unrelated).

@rfourquet
Copy link
Member Author

bump

@StefanKarpinski StefanKarpinski merged commit 3ff548a into master Jul 11, 2017
@StefanKarpinski StefanKarpinski deleted the rf/big-base branch July 11, 2017 13:59
@iblislin
Copy link
Member

FreeBSD CI builds failed after this PR merged https://julia.iblis.cnmc.tw/#/builders/1/builds/25

@rfourquet
Copy link
Member Author

Ouch, it seems the len field of String disappeared in the last month... maybe 2b47865 ? it seems sizeof is the alternative, will make a patch.

iblislin
iblislin reviewed Jul 11, 2017
View reviewed changes
base/gmp.jl
write(buf, s)
String(buf)
isneg(n) && unsafe_store!(ptr, '-' % UInt8)
str.len -= 1 # final '\0'
Copy link
Member

Choose a reason for hiding this comment

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

The stacktrace point out this line

Copy link
Contributor

Choose a reason for hiding this comment

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

Good that the len field is removed....

@timholy timholy mentioned this pull request Jul 11, 2017
Merged
rfourquet added a commit that referenced this pull request Jul 11, 2017
@rfourquet
remove the use of String's len field (introduced in #22133)
dda1b77
rfourquet added a commit that referenced this pull request Jul 11, 2017
@rfourquet
remove the use of String's len field (introduced in #22133)
ba16e8b
rfourquet added a commit that referenced this pull request Jul 11, 2017
@rfourquet
remove the use of String's len field (introduced in #22133)
7525b08
rfourquet added a commit that referenced this pull request Jul 11, 2017
@rfourquet
remove the use of String's len field (introduced in #22133)
770ad1e
JeffBezanson added a commit that referenced this pull request Jul 11, 2017
@JeffBezanson
Merge pull request #22756 from JuliaLang/rf/fix-str-len
37ca8c7 
remove the use of String's len field (introduced in #22133)
@tkelman
Copy link
Contributor

tkelman commented Jul 11, 2017

Before merging, should really check that the CI result isn't so old as to be obsolete when a PR hasn't had any recent activity.

@StefanKarpinski
Copy link
Sponsor Member

Yeah, sorry about that.

@timholy
Copy link
Sponsor Member

timholy commented Jul 11, 2017

Unless we want to re-run every PR every 24 hrs, this stuff is just going to happen sometimes.

Best lesson is to merge good work quickly. We need an army of @tkelman(s) with their insightful eyes on everything all the time 😄.

@tkelman
Copy link
Contributor

tkelman commented Jul 11, 2017

We can re-run the ones that are ready to merge before doing so, if it's been more than a few days since CI last ran.

@KristofferC
Copy link
Sponsor Member

This either has to be automated a la Rust or it will keep happening.

@tkelman
Copy link
Contributor

tkelman commented Jul 12, 2017

homu does not support appveyor barosl/homu#87

for now please put a bit of thought into how old the CI results are before you merge something.

@rfourquet rfourquet mentioned this pull request Jul 17, 2017
Merged
jeffwong pushed a commit to jeffwong/julia that referenced this pull request Jul 24, 2017
@rfourquet @jeffwong
remove the use of String's len field (introduced in JuliaLang#22133)
1e030ab
ararslan pushed a commit that referenced this pull request Sep 11, 2017
@StefanKarpinski @ararslan
Merge pull request #22133 from JuliaLang/rf/big-base
41ecf54 
fix base(b, big(0), 0) == "0"

Ref #22133
(cherry picked from commit 3ff548a)
ararslan pushed a commit that referenced this pull request Sep 13, 2017
@rfourquet @ararslan
fix base(b, big(0), 0) == "0"
7d088ba 
Ref #22133
(cherry picked from commit 994f42c)
vtjnash pushed a commit that referenced this pull request Sep 14, 2017
@rfourquet @vtjnash
fix base(b, big(0), 0) == "0"
2e9b973 
Ref #22133
(cherry picked from commit 994f42c)
ararslan pushed a commit that referenced this pull request Sep 15, 2017
@rfourquet @ararslan
fix base(b, big(0), 0) == "0"
29cc5b0 
Ref #22133
(cherry picked from commit 994f42c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuyichao yuyichao yuyichao left review comments

@iblislin iblislin iblislin left review comments

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

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

Successfully merging this pull request may close these issues.

None yet
8 participants
@rfourquet @iblislin @tkelman @StefanKarpinski @timholy @KristofferC @yuyichao @ararslan