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

Reserve more numeric types. #907

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@mahkoh
Contributor

mahkoh commented Feb 25, 2015

@mahkoh

This comment has been minimized.

Show comment
Hide comment
@mahkoh

mahkoh Feb 25, 2015

Contributor

Previous discussion: #138

Contributor

mahkoh commented Feb 25, 2015

Previous discussion: #138

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Feb 25, 2015

Member

What is this protecting against? That is, what's a program that will stop compiling if we don't reserve them?

It seems to me (and the filer of the original RFC) that shadowing should mean things work fine.

Member

huonw commented Feb 25, 2015

What is this protecting against? That is, what's a program that will stop compiling if we don't reserve them?

It seems to me (and the filer of the original RFC) that shadowing should mean things work fine.

@mahkoh

This comment has been minimized.

Show comment
Hide comment
@mahkoh

mahkoh Feb 25, 2015

Contributor

That is, what's a program that will stop compiling if we don't reserve them?

As mentioned in the RFC:

<anon>:1:1: 1:15 error: user-defined types or type parameters cannot shadow the primitive types [E0317]
<anon>:1 type u8 = u16;
         ^~~~~~~~~~~~~~
Contributor

mahkoh commented Feb 25, 2015

That is, what's a program that will stop compiling if we don't reserve them?

As mentioned in the RFC:

<anon>:1:1: 1:15 error: user-defined types or type parameters cannot shadow the primitive types [E0317]
<anon>:1 type u8 = u16;
         ^~~~~~~~~~~~~~
# Detailed design
Reserve the following type names: `fN`, `uN`, `iN` for `N` a multiple of 8.

This comment has been minimized.

@huonw

huonw Feb 25, 2015

Member

What limit? Or just to, say, 232?

@huonw

huonw Feb 25, 2015

Member

What limit? Or just to, say, 232?

This comment has been minimized.

@mahkoh

mahkoh Feb 25, 2015

Contributor

Whatever the parser can handle.

@mahkoh

mahkoh Feb 25, 2015

Contributor

Whatever the parser can handle.

This comment has been minimized.

@huonw

huonw Feb 25, 2015

Member

The parser can handle arbitrarily large numbers: congruence mod 8 can be determined using only the last 3 digits of a number represented in base 10, meaning string handling works fine, and hence i1234567891234567891234567891234567800 could be checked to be "valid" under that rule.

@huonw

huonw Feb 25, 2015

Member

The parser can handle arbitrarily large numbers: congruence mod 8 can be determined using only the last 3 digits of a number represented in base 10, meaning string handling works fine, and hence i1234567891234567891234567891234567800 could be checked to be "valid" under that rule.

This comment has been minimized.

@jmesmon

jmesmon Mar 3, 2015

Is there any reason to limit ourselves to numbers that are a multiple of 8? Why not just reserve them for all numbers?

@jmesmon

jmesmon Mar 3, 2015

Is there any reason to limit ourselves to numbers that are a multiple of 8? Why not just reserve them for all numbers?

@Thiez

This comment has been minimized.

Show comment
Hide comment
@Thiez

Thiez Feb 25, 2015

Why take only multiples of 8 for N? If this change is going to be made we might as well take all of them, LLVM has some support for this.

Thiez commented Feb 25, 2015

Why take only multiples of 8 for N? If this change is going to be made we might as well take all of them, LLVM has some support for this.

@cmr

This comment has been minimized.

Show comment
Hide comment
@cmr

cmr Mar 2, 2015

Member

That's a pretty annoying shadowing restriction right there, though it does avoid exposing the resolve bugs around such things...

Member

cmr commented Mar 2, 2015

That's a pretty annoying shadowing restriction right there, though it does avoid exposing the resolve bugs around such things...

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Mar 2, 2015

Contributor

@cmr
This restriction is temporary and will be removed if @eddyb 's plan works
See rust-lang/rust#20427 (comment)

Contributor

petrochenkov commented Mar 2, 2015

@cmr
This restriction is temporary and will be removed if @eddyb 's plan works
See rust-lang/rust#20427 (comment)

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Mar 3, 2015

Member

👎 to this RFC as written, it's a really large hammer and only makes sense if we had arbitrarily sized primitives (like iN in LLVM, but we don't expose that - not that backends can deal with uncommon bit widths, anyways).

The only approach I know of and I consider (more or less) "correct" would be the one mentioned in the comment linked by @petrochenkov above.

That would protect against breakage by explicitly having all existing built-in types part of prelude::v1, which means f128 can be added backwards-compatibly, by requiring it to be manually imported from std::num (and only show up in prelude::v2, if ever).

That said, I almost forgot about it, and it requires associated constants to be done backwards-compatibly (or as close as we can get to that).

Member

eddyb commented Mar 3, 2015

👎 to this RFC as written, it's a really large hammer and only makes sense if we had arbitrarily sized primitives (like iN in LLVM, but we don't expose that - not that backends can deal with uncommon bit widths, anyways).

The only approach I know of and I consider (more or less) "correct" would be the one mentioned in the comment linked by @petrochenkov above.

That would protect against breakage by explicitly having all existing built-in types part of prelude::v1, which means f128 can be added backwards-compatibly, by requiring it to be manually imported from std::num (and only show up in prelude::v2, if ever).

That said, I almost forgot about it, and it requires associated constants to be done backwards-compatibly (or as close as we can get to that).

@bombless

This comment has been minimized.

Show comment
Hide comment
@bombless

bombless Mar 3, 2015

We should fix the bug itself, not to shadow it with strange behavior.

bombless commented Mar 3, 2015

We should fix the bug itself, not to shadow it with strange behavior.

@alexcrichton alexcrichton self-assigned this Mar 5, 2015

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 5, 2015

Member

Thanks for the RFC! After some discussion, however, we don't think that this is the strategy that we would want to take. It is arguably a resolve bug which prevents shadowing primitives and we can consider it a requirement that adding a type like f16 would require fixing the resolve bug first to allow shadowing of primitives. As such the change should still be backwards compatible, just perhaps a little harder than before, so I'm going to close this.

Member

alexcrichton commented Mar 5, 2015

Thanks for the RFC! After some discussion, however, we don't think that this is the strategy that we would want to take. It is arguably a resolve bug which prevents shadowing primitives and we can consider it a requirement that adding a type like f16 would require fixing the resolve bug first to allow shadowing of primitives. As such the change should still be backwards compatible, just perhaps a little harder than before, so I'm going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment