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

#11105 Add syntax errors for unsigned literals > UInt128 #11130

Merged
merged 1 commit into from
May 6, 2015

Conversation

ScottPJones
Copy link
Contributor

Previously, these unsigned literals would get promoted from an unsigned integer to a signed type,
which could lead to errors.
It is safer to give a syntax error, while the user can still explicitly get a BigInt value using hex, octal or binary notation by doing big"0x...." instead.

@pao
Copy link
Member

pao commented May 5, 2015

cf #11105

In order to get the auto-close behavior, the commit message needs the word "fixes" or "closes" before the cross-reference. It's generally better to put that in the descriptive part of the commit message rather than the title line (but that's not a rigorous rule).

Probably needs a check over from @JeffBezanson since it touches the parser, and cc @jakebolewski as a heads up for JuliaParser.jl.

@ScottPJones
Copy link
Contributor Author

@pao Thanks for the continued advice! I actually don't even know what you mean by the auto-close behavior, or how to separate out a title line from a descriptive part of a commit message... I just do git commit -a -m "message"

@pao
Copy link
Member

pao commented May 5, 2015

If you have a "reasonable" git installation, for some definition of reasonable, you will get an editor pop up if you leave off the -m "message", which allows you to enter a more detailed commit message:

parser: syntax error for large hex literals #try to keep this short for git log --oneline

Modifies the parser to throw a syntax error once a hex integer literal cannot
be represented as a `Uint128`, and tests for that behavior.

Fixes #11105.

With the "fixes"/"closes"/"fix" bit in there, if this pull request is merged, the other issue will automatically be closed, which besides adding traceability is rather convenient.

@ScottPJones
Copy link
Contributor Author

Hmmm.... something must be wrong with my installation then... it brings up Aquamacs, but the git commit command has already ended with an error...

ScottsRetinaMBP:j scott$ git commit -a
Aborting commit due to empty commit message.

@pao
Copy link
Member

pao commented May 5, 2015

#@test -0o0000000000000000000000000000000000000000001 ==
# -(0o0000000000000000000000000000000000000000001)
#@test -0b000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001 ==
# -(0b000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001)
Copy link
Member

Choose a reason for hiding this comment

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

Seems like just deleting these would be better, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, sure, will do... thanks!

@jakebolewski
Copy link
Member

@ScottPJones this should throw a deprecation warning, not an error at this point (to follow our deprecation policy). See the other instances of throwing deprecated syntax warnings in this file.

The tests can also be removed completely.

@StefanKarpinski
Copy link
Member

@jakebolewski, I kind of doubt that there's much code relying on this though – having hex BigInt literals is pretty strange (which is why we're removing it), and it would never happen at runtime since it is a parse time error.

@jakebolewski
Copy link
Member

Why not deprecate the behavior of parsing signed BigInt's as well as unsigned ones? It would be great to remove the BigInt dependency from the frontend.

@tkelman
Copy link
Contributor

tkelman commented May 5, 2015

Why not deprecate the behavior of parsing signed BigInt's as well as unsigned ones? It would be great to remove the BigInt dependency from the frontend.

I'd be greatly in favor of that, but that probably doesn't need to be done within the same PR?

@jakebolewski
Copy link
Member

No you are right, but if we were going to deprecate parsing BigInt literals, it would make sense to do so for both the signed and unsigned forms.

@StefanKarpinski
Copy link
Member

Let's stick with one thing at a time – everyone agrees that parsing long unsigned literals as BigInts is a bad idea, so let's delete it. I for one, am not convinced yet that we want to get rid of parsing large signed literals as BigInts.

@ScottPJones
Copy link
Contributor Author

I agree with @StefanKarpinski on not (yet) being convinced of getting rid of parsing large signed literals... I mainly brought this up because of the inconsistency and potential surprise of the unsigned to signed types... but I also understand @jakebolewski 's concern about dependencies... since there is always the option of using big"... very long signed literal ...", and I doubt anyone (except the test cases!) are actually depending on long signed literals... maybe I'd lean (in another PR, of course), to removing all conversions to big types (what about conversions to BigFloats? ;-) I don't understand there why there's a Int128, but no "double double" Float128 type... underlying compiler support?

I'll remove the old tests, change to deprecation warnings, possibly fix up the tests I added (how do you test for a deprecation warning? I know @test and @test_throws so far...), hopefully this PR will be good to go then...

Thanks everybody!

@jakebolewski
Copy link
Member

Thanks @ScottPJones for the update. There has been talk of Float80 / Float128 support (linking to GNU libquadmath). See issue #757.

jakebolewski added a commit that referenced this pull request May 6, 2015
#11105 Add syntax errors for unsigned literals > UInt128
@jakebolewski jakebolewski merged commit b6904b6 into JuliaLang:master May 6, 2015
jakebolewski added a commit that referenced this pull request May 6, 2015
mbauman pushed a commit to mbauman/julia that referenced this pull request Jun 6, 2015
tkelman pushed a commit to tkelman/julia that referenced this pull request Jun 6, 2015
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