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

Added support for struct-like enum variants in middle::ty::enum_variants() #7557

Merged
merged 1 commit into from Jul 7, 2013

Conversation

Projects
None yet
5 participants
@michaelwoerister
Contributor

michaelwoerister commented Jul 2, 2013

After getting an ICE trying to use the Repr enum from middle::trans::adt (see issue #7527), I tried to implement the missing case for struct-like enum variants in middle::ty::enum_variants(). It seems to work now (and passes make check) but there are still some uncertainties that bother me:

  • I'm not sure I did everything, right. Especially getting the variant constructor function from the variant node id is just copied from the tuple-variant case. Someone with more experience in the code base should be able to see rather quickly whether this OK so.
  • It is kind of strange that I could not reproduce the ICE with a smaller test case. The unimplemented code path never seems to be hit in most cases, even when using the exact same Repr enum, just with ty::t replaced by an opaque pointer. Also, within the adt module, Repr and matching on it is used multiple times, again without running into problems. Can anyone explain why this is the case? That would be much appreciated.

Apart from that, I hope this PR is useful.

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 2, 2013

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 3, 2013

See whether this fixes the testcase from #6362?

@pcwalton

This comment has been minimized.

pcwalton commented on 866a5b1 Jul 3, 2013

r+

This comment has been minimized.

thestinger replied Jul 4, 2013

@bors: retry

This comment has been minimized.

jdm replied Jul 6, 2013

@bors: retry

This comment has been minimized.

jdm replied Jul 7, 2013

@bors: retry

@bors

This comment has been minimized.

Contributor

bors commented on 866a5b1 Jul 4, 2013

saw approval from pcwalton
at michaelwoerister@866a5b1

This comment has been minimized.

Contributor

bors replied Jul 4, 2013

merging michaelwoerister/rust/enum_structs = 866a5b1 into auto

This comment has been minimized.

Contributor

bors replied Jul 4, 2013

michaelwoerister/rust/enum_structs = 866a5b1 merged ok, testing candidate = 880986ab

This comment has been minimized.

Contributor

bors replied Jul 5, 2013

saw approval from pcwalton
at michaelwoerister@866a5b1

This comment has been minimized.

Contributor

bors replied Jul 7, 2013

saw approval from pcwalton
at michaelwoerister@866a5b1

This comment has been minimized.

Contributor

bors replied Jul 7, 2013

merging michaelwoerister/rust/enum_structs = 866a5b1 into auto

This comment has been minimized.

Contributor

bors replied Jul 7, 2013

michaelwoerister/rust/enum_structs = 866a5b1 merged ok, testing candidate = 88487d8

This comment has been minimized.

Contributor

bors replied Jul 7, 2013

fast-forwarding master to auto = 88487d8

bors added a commit that referenced this pull request Jul 5, 2013

auto merge of #7557 : michaelwoerister/rust/enum_structs, r=pcwalton
After getting an ICE trying to use the `Repr` enum from middle::trans::adt (see issue #7527), I tried to implement the missing case for struct-like enum variants in `middle::ty::enum_variants()`. It seems to work now (and passes make check) but there are still some uncertainties that bother me:
+ I'm not sure I did everything, right. Especially getting the variant constructor function from the variant node id is just copied from the tuple-variant case. Someone with more experience in the code base should be able to see rather quickly whether this OK so.
+ It is kind of strange that I could not reproduce the ICE with a smaller test case. The unimplemented code path never seems to be hit in most cases, even when using the exact same `Repr` enum, just with `ty::t` replaced by an opaque pointer. Also, within the `adt` module, `Repr` and matching on it is used multiple times, again without running into problems. Can anyone explain why this is the case? That would be much appreciated. 

Apart from that, I hope this PR is useful.

bors added a commit that referenced this pull request Jul 7, 2013

auto merge of #7557 : michaelwoerister/rust/enum_structs, r=pcwalton
After getting an ICE trying to use the `Repr` enum from middle::trans::adt (see issue #7527), I tried to implement the missing case for struct-like enum variants in `middle::ty::enum_variants()`. It seems to work now (and passes make check) but there are still some uncertainties that bother me:
+ I'm not sure I did everything, right. Especially getting the variant constructor function from the variant node id is just copied from the tuple-variant case. Someone with more experience in the code base should be able to see rather quickly whether this OK so.
+ It is kind of strange that I could not reproduce the ICE with a smaller test case. The unimplemented code path never seems to be hit in most cases, even when using the exact same `Repr` enum, just with `ty::t` replaced by an opaque pointer. Also, within the `adt` module, `Repr` and matching on it is used multiple times, again without running into problems. Can anyone explain why this is the case? That would be much appreciated. 

Apart from that, I hope this PR is useful.

@bors bors closed this Jul 7, 2013

@bors bors merged commit 866a5b1 into rust-lang:master Jul 7, 2013

1 check passed

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