Refactor fallibility of conversions from IR to Rust types #581
+418
−150
Conversation
This file contains hidden or 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
|
|
fitzgen
commented
Mar 14, 2017
src/codegen/mod.rs
Outdated
| primitive_ty(ctx, "isize") | ||
| } | ||
| _ => return None, | ||
| }) |
Member
Author
There was a problem hiding this comment.
Note to self: remove this leftover from rebasing away a bad cargo fmt.
Member
Author
|
Also, I should add the original test case from #573 (comment) to our test suite. |
emilio
approved these changes
Mar 15, 2017
Contributor
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
r=me with the documented variants and the cleaned up rustfmt code.
Thanks!
| /// Errors that can occur during code generation. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum Error { | ||
| NoLayoutForOpaqueBlob, |
Contributor
There was a problem hiding this comment.
Document the variants, please :)
|
|
||
| /// Errors that can occur during code generation. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum Error { |
Contributor
There was a problem hiding this comment.
This seems like an overly generic name for a really specific error kind. I don't have a clearly better name in mind though.
Member
Author
There was a problem hiding this comment.
It is namespaced, however. I hope that it can become the error type for all fallible codegen operations.
fitzgen
force-pushed
the
issue-573-even-more-spidermonkey-layout-test-failures
branch
from
30267de to
b312c9c
Compare
Member
Author
|
@bors-servo r=emilio |
|
📌 Commit b312c9c has been approved by |
|
🔒 Merge conflict |
|
☔ The latest upstream changes (presumably #583) made this pull request unmergeable. Please resolve the merge conflicts. |
The old ToRustTy and ItemToRustTy conversion traits were problematic
in that they assumed infallibity. That assumption is problematic
because we are often dealing with C++ constructs for which Rust has no
equivalent *and* we don't have a usable layout from which to generate
an opaque blob in its stead. But, a usable layout is often "up the
stack" if only we had a way to get up there.
For example, Rust does not currently have an equivalent of const value
template parameters, and libclang will not give us a layout for
template definitions with const value template parameters. However,
libclang will give us the layout for an instantiation of such a
template definition.
First, this commit separates the concepts of generating an equivalent
Rust type from generating an opaque blob of the same size and
alignment as an IR thing. This consolidates and DRYs up a *ton* of
code involved in falling back to an opaque blob (and doing our best to
get a Layout for the blob) when we can't generate an equivalent Rust
type for some IR thing.
Second, it separates fallible and infallible conversions, and provides
a nice little framework for when to choose which. This gives us one
single place where we do this whole dance:
if could not generate equivalent Rust type:
if we have a layout:
return opaque blob based on layout
else:
return opaque blob based on a wish and a prayer
The ToRustTy trait is replaced by the TryToOpaque, ToOpaque,
TryToRustTyOrOpaque, and ToRustTyOrOpaque traits. The ItemToRustTy
helper was just a way to avoid ToRustTy's Self::Extra parameter when
it was not needed, and is simply removed without a replacement. We
suck it up and pass `&()` at call sites now. We *could* introduce
ItemTryToOpaque, ItemToOpaque, etc... traits, but the cost of the
added boiler plate would outweigh the benefits of not passing `&()` at
call sites, IMHO.
In addition to being a nice code cleanup, this also fixes rust-lang#573.
fitzgen
force-pushed
the
issue-573-even-more-spidermonkey-layout-test-failures
branch
from
b312c9c to
92c1a25
Compare
Member
Author
|
@bors-servo r=emilio |
|
📌 Commit 92c1a25 has been approved by |
bors-servo
pushed a commit
that referenced
this pull request
Mar 15, 2017
…test-failures, r=emilio
Refactor fallibility of conversions from IR to Rust types
The old `ToRustTy` and `ItemToRustTy` conversion traits were problematic in that they assumed infallibity. That assumption is problematic because we are often dealing with C++ constructs for which Rust has no equivalent *and* we don't have a usable layout from which to generate an opaque blob in its stead. But, a usable layout is often "up the stack" if only we had a way to get up there.
For example, Rust does not currently have an equivalent of const value template parameters, and libclang will not give us a layout for template definitions with const value template parameters. However, libclang will give us the layout for an instantiation of such a template definition.
First, this commit separates the concepts of generating an equivalent Rust type from generating an opaque blob of the same size and alignment as an IR thing. This consolidates and DRYs up a *ton* of code involved in falling back to an opaque blob (and doing our best to get a `Layout` for the blob) when we can't generate an equivalent Rust type for some IR thing.
Second, it separates fallible and infallible conversions, and provides a nice little framework for when to choose which. This gives us one single place where we do this whole dance:
if could not generate equivalent Rust type:
if we have a layout:
return opaque blob based on layout
else:
return opaque blob based on a wish and a prayer
The `ToRustTy` trait is replaced by the `TryToOpaque`, `ToOpaque`, `TryToRustTyOrOpaque`, and `ToRustTyOrOpaque` traits. The `ItemToRustTy` helper was just a way to avoid `ToRustTy`'s `Self::Extra` parameter when it was not needed, and is simply removed without a replacement. We suck it up and pass `&()` at call sites now. We *could* introduce `ItemTryToOpaque`, `ItemToOpaque`, etc... traits, but the cost of the added boiler plate would outweigh the benefits of not passing `&()` at call sites, IMHO.
In addition to being a nice code cleanup, this also fixes #573.
r? @emilio
|
☀️ Test successful - status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The old
ToRustTyandItemToRustTyconversion traits were problematic in that they assumed infallibity. That assumption is problematic because we are often dealing with C++ constructs for which Rust has no equivalent and we don't have a usable layout from which to generate an opaque blob in its stead. But, a usable layout is often "up the stack" if only we had a way to get up there.For example, Rust does not currently have an equivalent of const value template parameters, and libclang will not give us a layout for template definitions with const value template parameters. However, libclang will give us the layout for an instantiation of such a template definition.
First, this commit separates the concepts of generating an equivalent Rust type from generating an opaque blob of the same size and alignment as an IR thing. This consolidates and DRYs up a ton of code involved in falling back to an opaque blob (and doing our best to get a
Layoutfor the blob) when we can't generate an equivalent Rust type for some IR thing.Second, it separates fallible and infallible conversions, and provides a nice little framework for when to choose which. This gives us one single place where we do this whole dance:
The
ToRustTytrait is replaced by theTryToOpaque,ToOpaque,TryToRustTyOrOpaque, andToRustTyOrOpaquetraits. TheItemToRustTyhelper was just a way to avoidToRustTy'sSelf::Extraparameter when it was not needed, and is simply removed without a replacement. We suck it up and pass&()at call sites now. We could introduceItemTryToOpaque,ItemToOpaque, etc... traits, but the cost of the added boiler plate would outweigh the benefits of not passing&()at call sites, IMHO.In addition to being a nice code cleanup, this also fixes #573.
r? @emilio