Move numeric consts to associated consts step1 #68325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
dtolnay
reviewed
Jan 19, 2020
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
faern
force-pushed
the
move-numeric-consts-to-associated-consts-step1
branch
from
e0e3d23 to
bd51e02
Compare
|
@dtolnay Adding an associated constant directly on a type which implements a trait that has an associated constant with the same name (in scope) silently makes So for this PR/RFC that means anyone implementing traits on the primitive numeric types that have constants with the same names as the ones we add will now get our new values unless they properly wrote I have not done any research into how common that might be downstream. I have no idea how I would do that except for a crater run? I hope/assume it would not be a common problem. Because even if some people have defined traits with these constants on the primitive types they are very likely the same type and the same value, it would not make sense to add these constants with different values really. And if they do have different types (like the errors I had to fix in this PR) then they will at least get a compilation error and not silently new values. The exact same thing happens if you add a function to a type that has a function with the same name defined in a trait. Neither of these can be considered breaking(?!) because that would mean you could never add constants or functions to any type, because you risk overriding similar items on traits implemented by that type. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Could someone help me fix/understand the broken |
faern
force-pushed
the
move-numeric-consts-to-associated-consts-step1
branch
2 times, most recently
from
0880ef2 to
b9831b7
Compare
|
Fixed. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
faern
force-pushed
the
move-numeric-consts-to-associated-consts-step1
branch
2 times, most recently
from
9f2ff22 to
1ef705d
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
faern
force-pushed
the
move-numeric-consts-to-associated-consts-step1
branch
from
1ef705d to
8c865e2
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
The potential breakage is indeed not very nice. I'm pretty sure this is covered by RFC 1105 API Evolution and thus allowed. We should still take care, run crater and decide in the team. We'll do that once PR is green.
|
☀️ Try build successful - checks-azure |
|
Seems to pass. So do we want a crater run on this before merging? |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@Mark-Simulacrum I think crater has become stuck, its percentage of completion hasn't updated all day. |
|
Yeah, it does that unfortunately. Cc @pietroalbini |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
Sounds pretty optimal to me :) Merge? |
|
Yep, that sounds optimal indeed. Adding inherent items is considered a minor change by RFC 1105. The particular items here are unlikely to be added by user code and even more unlikely to be added with a different meaning. I was thinking this might clash with With all that, I think the teams approval is not necessary in this case. @bors r+ |
|
📌 Commit 61fecfb has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 61fecfb with merge 2315451ad6ec1c90d93edf658ead2eaf7295cd08... |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit 61fecfb has been approved by |
bors
added a commit
that referenced
this pull request
Jan 30, 2020
…-step1, r=LukasKalbertodt Move numeric consts to associated consts step1 A subset of #67913. Implements the first step of RFC rust-lang/rfcs#2700 This PR adds the new constants as unstable constants and defines the old ones in terms of the new ones. Then fix a tiny bit of code that started having naming collisions because of the new assoc consts. Removed a test that did not seem relevant any longer. Since doing just `u8::MIN` should now indeed be valid.
|
Today I learned: do not edit your comment with the |
|
☀️ Test successful - checks-azure |
bors
added a commit
that referenced
this pull request
Mar 4, 2020
Stabilize assoc_int_consts associated int/float constants The next step in RFC rust-lang/rfcs#2700 (tracking issue #68490). Stabilizing the associated constants that were added in #68325. * Stabilize all constants under the `assoc_int_consts` feature flag. * Update documentation on old constants to say they are soft-deprecated and the new ones should be preferred. * Update documentation examples to use new constants. * Remove `uint_macro` and use `int_macro` for all integer types since the macros were identical anyway. r? @LukasKalbertodt
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A subset of #67913. Implements the first step of RFC rust-lang/rfcs#2700
This PR adds the new constants as unstable constants and defines the old ones in terms of the new ones. Then fix a tiny bit of code that started having naming collisions because of the new assoc consts.
Removed a test that did not seem relevant any longer. Since doing just
u8::MINshould now indeed be valid.