Algebraic Datatype Renovation #5183
Closed
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
|
Exciting. I'll take a look. |
|
OK. So I read through the patches. I think this is great. I would like to see the various comments I mentioned above added in, but otherwise I think it's good to go. |
|
And I think that's all I'm going to stack on this branch as opposed to doing as a followup. |
|
It doesn't merge 100% cleanly anymore, so I have to push a rebase. (I'll keep cc0a88c live, in case anyone wants to do a second-order diff to inspect it; it's just a couple of context changes.) |
|
Don't merge this right now; the rebased version broke the rustpkg tests in a strange way that I don't fully understand and that seems to have something to do with the const enum vector of tests(!). |
|
The bug exists prior to this branch, but a slight change to how enum padding is computed means that it breaks tests instead of going unnoticed. Filed #5210. |
|
Okay. It should work now. |
…sion. Later changes on this branch adapt the rest of rustc::middle::trans to use this module instead of scattered hard-coded knowledge of representations; a few of them also have improvements or cleanup for adt.rs (and many added comments) that weren't drastic enough to justify changing history to move them into this commit.
It causes an LLVM assertion for every host/target word-size combination on incoming at the time of this writing.
This change remains separate from the addition of adt.rs, even though it's necessary for compatibility with pre-trans::adt representation, to serve as an example of a change to the representation logic.
This, like the previous change for unit-like enums, is only necessary until everything is fully converted to use trans::adt.
The first is reduced from a case in rustdoc (originally involving an ARC); the other is related. No committed version has gotten these wrong, but when I broke them it showed up only in rustdoc; there was nothing in the test suite (or the compiler!) that failed. The general issue is that the statics and trans have to agree on order of evaluation, or else you get use-after-move-out-of errors at runtime.
Note that in the ByValue case (which can't happen? yet?) we're still effectively bitcasting, I think. So this change adds a way to assert that that's safe. Note also, for future reference, that LLVM's instcombine pass will turn a bitcast into a GEP(0, 0, ...) if possible.
Also converts const cast-from-enum, because it used the same routine to get the discriminant as what's renovated to construct the enums. Also fixes ICE on struct-like variants as consts, and provides a slightly less bad ICE for functional-update-like struct expressions in consts.
As a result, trans_enum_variant no longer cares what kind of enum it's dealing with, so the "degen" parameter goes away in a bunch of places.
Surely this cannot be the best way to get the type.
This way "field" refers to the abstraction and "element" (as in get_elt, "get element pointer", etc.) refers to the low-level LLVM operations.
|
Diff of the actual changes made in the last round, 92b88a1..a69ec17, as a gist because apparently Github only does three-dot diff ranges: https://gist.github.com/jld/c945835fcc861c6b9282 There's a fix for the conflict with 7921810, a fix for some code that was wrong but unreachable until that fix, and a minor issue or two that I'm pretty sure only broke intermediate states along the branch (which I tripped over while trying to test my actual fixups). |
bors
added a commit
that referenced
this pull request
Mar 7, 2013
This series of changes moves the representation details of algebraic datatypes (enums, and special cases like structs and tuples and (until they're fully removed) records) from various places around rustc::middle::trans into a single module, to enable future improvements in this area. r?(@nikomatsakis), and the core developers in general; this seems like a “super-review” kind of change.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 2, 2020
Don't lint `single_component_path_imports` in macros Fixes rust-lang#5154 changelog: Fix false positive in `single_component_path_imports`
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 2, 2020
…shearth,llogiq,flip1995 I like to move it, move it GHA now runs in the background for 6 days (rust-lang#5088) Since then ~~15~~ 19 PRs were successfully merged and Travis+Appveyor agreed on the status in every case. ([GitHub PR search query](https://github.com/rust-lang/rust-clippy/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged+merged%3A%3E%3D2020-02-12T15%3A42%3A00+sort%3Aupdated-desc+NOT+%5Bgh-pages%5D+in%3Atitle)) Some PRs were: - rust-lang#5163 - rust-lang#5170 - rust-lang#5168 - rust-lang#5173 - rust-lang#5171 - rust-lang#5156 - rust-lang#4809 - rust-lang#5177 - rust-lang#5182 - rust-lang#5183 - rust-lang#5184 - rust-lang#5185 - rust-lang#5186 - rust-lang#5181 - rust-lang#5189 Bug with GHA: - When a rustc PR gets merged between the `integration_build` and the `integration` job, the `integration` job will fail. This happened once in rust-lang#5162, but not in the past 6 days. Even if it would happen every 4th PR we would save time, since splitting up the integration build and tests saves 5-7 minutes per run and a complete run takes 15-17 minutes - Sometimes the MacOS build takes up to an hour to download the master toolchain. Until now, this happend 2 or 3 times and can be resolved by a `@bors r3try`+canceling the previous run (restarting single jobs is not supported yet) ## Before merging this, rust-lang/rust-central-station#578 has to get merged This PR is for starting the discussion and to get consensus (@rust-lang/clippy) on a final move to GHA. If we're ready, I'll contact Pietro, to finalize the move. changelog: Clippy completely runs on GHA now 🎉 --- BTW: The deployment already runs on GHA, instead of Travis.
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.
This series of changes moves the representation details of algebraic datatypes (enums, and special cases like structs and tuples and (until they're fully removed) records) from various places around rustc::middle::trans into a single module, to enable future improvements in this area.
r?(@nikomatsakis), and the core developers in general; this seems like a “super-review” kind of change.