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

RFC: Fixes issue 3453, string literal input formats for Int128 & Uint128 #4813

Merged
merged 14 commits into from
Dec 15, 2013

Conversation

nanosec
Copy link

@nanosec nanosec commented Nov 14, 2013

See #3453. string literal input formats for Int128 & Uint128.

@pao
Copy link
Member

pao commented Nov 14, 2013

engaging autolinker: #3453

@JeffBezanson
Copy link
Sponsor Member

I would just remove the quoting, so the conversion from string to int is done at macro-expand time.

@StefanKarpinski
Copy link
Sponsor Member

Is this a finished pull request, or should we expect parser changes to emit macro calls when large integer literals occur in Julia source?

@nanosec
Copy link
Author

nanosec commented Nov 20, 2013

I originally thought of this as finished, but I've recently been making progress in changing the parser. I'm hoping to push changes in the next day or so. I guess I should have labeled this as a WIP instead.

@StefanKarpinski
Copy link
Sponsor Member

No problem. Just wanted to be sure. Looking forward to the rest.

@nanosec
Copy link
Author

nanosec commented Nov 26, 2013

Okay, I think I've finished the modifications. @StefanKarpinski, I ended up not needing to use the macros. I'm thinking of removing them, but would they be useful in other contexts?

@StefanKarpinski
Copy link
Sponsor Member

Oo! Impressive work. Taking a look. cc: @JeffBezanson

@StefanKarpinski
Copy link
Sponsor Member

Ok, I think I found a bug:

julia> 106301099528559113985750943957626130432881762387162312
-106301099528559113985750943957626130432881762387162312

@StefanKarpinski
Copy link
Sponsor Member

And yes, I just typed a lot of random digits.

@nanosec
Copy link
Author

nanosec commented Nov 26, 2013

Hmm, I should have tested before I pushed. I've seen this bug before! I thought I fixed it. Taking another look.

@JeffBezanson
Copy link
Sponsor Member

I don't think parsing these as function calls is going to fly. :(123...321) for some digit strings then gives a function call expression, which is surprising to say the least.

Parsing Int128s directly is tractable, since this type is defined very early (in boot.jl) and is in Core. But BigInt requires a fair bit of the standard library, so using it in parsing is questionable.

@StefanKarpinski
Copy link
Sponsor Member

That's why I recommended emitting macro calls instead.

@nanosec
Copy link
Author

nanosec commented Nov 26, 2013

@StefanKarpinski Like this? (Sorry, I'm very much a beginner at all this.)

@StefanKarpinski
Copy link
Sponsor Member

No worries, this is good work and this is not an unusual amount of back-and-forth for a change like this.

@StefanKarpinski
Copy link
Sponsor Member

Here's another issue:

julia> -0x00000000000000001
ERROR: invalid base-10 digit '-' in "-0x00000000000000001"

julia> :(-0x00000000000000001)
:(-(@uint128_str "-0x00000000000000001"))

@StefanKarpinski
Copy link
Sponsor Member

Here's another one:

julia> 0b000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
0x00000000000000000000000000000000

julia> typeof(ans)
Uint128

Same thing with hex and octal versions.

@StefanKarpinski
Copy link
Sponsor Member

I should note that that's 0b followed by 129 zeros. Should that be an unsigned BigInt literal? I'm not even sure since there's no unsigned BigInt type.

@StefanKarpinski
Copy link
Sponsor Member

@JeffBezanson, any thoughts? I'm ready to pull the trigger on this.

@JeffBezanson
Copy link
Sponsor Member

BigInts in ASTs might be a problem since we can't save them in the system image due to their use of native pointers.

@StefanKarpinski
Copy link
Sponsor Member

Yeah, the BigInt literals are the ones I'm the least sold on. Saving them to the system image shouldn't be a problem unless we use this functionality while building the system image, right?

StefanKarpinski added a commit that referenced this pull request Dec 15, 2013
Fixes issue 3453, string literal input formats for Int128 & Uint128

Note that this requires at least a `make clean`.
@StefanKarpinski StefanKarpinski merged commit 68cf700 into JuliaLang:master Dec 15, 2013
@mschauer
Copy link
Contributor

I now have the trouble that random.jl during creation of sys.ji has a dependency with BigInt via Base.Random.uuid4().
Replacing

#    u &= 0xffffffffffff0fff3fffffffffffffff
#    u |= 0x00000000000040008000000000000000
    u &= ~(uint128(0xf000c)<<60)
    u |= uint128(0x40008)<<60

fixes this for me, so there might be a dependency hidden which is connected to integer parsing. Warning: 32 bit system here.

@StefanKarpinski
Copy link
Sponsor Member

Those aren't BigInts – they're Uint128 literals, which should work without any issues, but yes, I'm not too surprised that's not entirely hiccup-free on a 32-bit system. Can you open an issue and give more detail about the problem? Sorry that master is such a mess for you right now – hopefully we can get it sorted out soon.

@StefanKarpinski
Copy link
Sponsor Member

You're not the only one: c415d04#commitcomment-4870539.

@JeffBezanson
Copy link
Sponsor Member

It would really be better to implement literals entirely in the front end. On 64-bit systems we have strtoll which would make this straightforward, but 32-bit would be a problem.

@StefanKarpinski
Copy link
Sponsor Member

That feedback would have been helpful anytime in the past month.

@JeffBezanson
Copy link
Sponsor Member

The problem is there is no easy way to proceed with that approach, due to a general paucity of Int128 support throughout the stack. And for BigInt the situation is even worse. The implementation of BigInt literals we have now is the only thing that will work, aside from rewriting the parser in julia, or building BigInts deeper into the system. And even then there is still the problem of saving the pointers inside them.

@StefanKarpinski
Copy link
Sponsor Member

So it sounds like this approach – since it actually works – was a good one. It appealed to me since it was minimally invasive, which is usually a pretty decent heuristic.

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.

5 participants