Skip to content
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

Factor out "value" variants of ConstValue #59210

Closed
oli-obk opened this issue Mar 15, 2019 · 10 comments · Fixed by #66233
Closed

Factor out "value" variants of ConstValue #59210

oli-obk opened this issue Mar 15, 2019 · 10 comments · Fixed by #66233
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2019

Basically create a new enum ConstKind which has all the variants of ConstValue except for Scalar, Slice, ByRef. ConstKind should then have a variant (maybe Evaluated or just Value), which contains a ConstValue.

cc #59178 (comment)

continuation of #54738

This is actually just some mechanical refactoring, but also a ton of work (since you'll be touching almost every occurrence of ConstValue).

@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Mar 15, 2019
This was referenced Mar 15, 2019
@RalfJung

This comment has been minimized.

@oli-obk

This comment has been minimized.

@stepnivlk
Copy link
Contributor

Hey @oli-obk, happy to take this one :).

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 15, 2019

Awesome! Please do note that this is a ton of work and may get annoying with rebases, so it may require some endurance to get actually merged.

@varkor
Copy link
Member

varkor commented Mar 19, 2019

It'd also be better to wait until #59008 is merged, because there's otherwise going to be a lot of conflicts.

@stepnivlk
Copy link
Contributor

@varkor I see, thx for the info.

@yodaldevoid
Copy link
Contributor

@stepnivlk If you did not see, #59008 got merged! Just wanted to ping you in case you are still interested in working on this.

@stepnivlk
Copy link
Contributor

@yodaldevoid great, thx for the info and I actually missed it. I'm def interested, gonna work on that.

@stepnivlk
Copy link
Contributor

hey @oli-obk I have one tiny question, @eddyb mentioned here #59178 (comment) that he wants ConstKind in ty and ConstValue in its current place, is that what we aim for? I'm asking b/c often it's necessary to bring in both ConstKind and ConstValue, mainly for pattern matching. From that perspective those two are quite coupled. I feel like it'd be cleaner to have them both together. What do you think?

@oli-obk
Copy link
Contributor Author

oli-obk commented May 13, 2019

ConstKind fits better to ty, since that's also where ty::Const is defined. It's perfectly fine that they live in different modules imo. Also, most of the time you won't import ConstKind, but use ty::ConstKind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants