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

Get the eval out of miri engine #54395

Closed
RalfJung opened this issue Sep 20, 2018 · 26 comments · Fixed by #61625
Closed

Get the eval out of miri engine #54395

RalfJung opened this issue Sep 20, 2018 · 26 comments · Fixed by #61625
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@RalfJung
Copy link
Member

I would prefer if this was named ConstEvalInterpreter or ConstEvalMachine to keep in the spirit of const_eval names while removing "eval" from everywhere in the interpret module.

So, in particular, EvalContext should become something else. InterpContext? InterpretContext?

The actual renaming is pretty easy to do, but there is a high change of conflicts with other PRs, so you should arrange the timing with @oli-obk and @RalfJung.

@RalfJung RalfJung added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Sep 20, 2018
@eddyb
Copy link
Member

eddyb commented Sep 20, 2018

InterpretCx is probably in the direction we'd want to go.

@Centril
Copy link
Contributor

Centril commented Sep 20, 2018

InterpCtx sounds optimal =P

@csmoe
Copy link
Member

csmoe commented Sep 21, 2018

I prefer InterpCtxt which is more consistent with TyCtxt for type context.

@Centril
Copy link
Contributor

Centril commented Sep 21, 2018

@csmoe I can agree to the extra 't' =P

@pvdrz
Copy link
Contributor

pvdrz commented Sep 21, 2018

Hi @RalfJung, I'd like to help in this one to get my feet wet. How/when shall we begin?

@RalfJung
Copy link
Member Author

We currently have #54380 and #53821 in flight and I have https://github.com/RalfJung/rust/tree/pointer-provenance lined up. We should wait for these to land, I think... which could take a bit at the current speed. :/

Until then, you could prepare a list of renames. Is there anything in librustc_mir::interpret that also has "eval" in its name?

@pvdrz
Copy link
Contributor

pvdrz commented Sep 21, 2018

Well, according to ripgrep we have EvalResult, EvalErrorKind and EvalSnapshot

@RalfJung
Copy link
Member Author

Sounds reasonable. So which new names do we want to pick? s/Eval/Interp/ sounds reasonable?

@pvdrz
Copy link
Contributor

pvdrz commented Sep 22, 2018

yeah that sounds reasonable

@eddyb
Copy link
Member

eddyb commented Sep 22, 2018

FWIW the "latest" convention, that I apparently forgot to try and convert the rest of the compiler to, is the Cx suffix for types.

@eddyb
Copy link
Member

eddyb commented Sep 22, 2018

@christianpoveda I'm not sure we needed the Eval prefix, except for EvalResult - can we do the io::Result thing here?

@pvdrz
Copy link
Contributor

pvdrz commented Sep 22, 2018

Sure :)

@pvdrz
Copy link
Contributor

pvdrz commented Nov 20, 2018

@RalfJung are we ready to do this? #54380 and #53821 have been merged.

@RalfJung
Copy link
Member Author

Things sure have calmed down a bit. You should probably work on top of #55915 though, that one is I think the most conflict-risky.

But we should first assemble a list of renames that we all are happy with. Which renames to you intend to do?

@pvdrz
Copy link
Contributor

pvdrz commented Nov 20, 2018

Sure, let me check the source code and I'll put a list

@kenta7777
Copy link
Contributor

I'd like to tackle this issue. How shall we begin?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 25, 2019

As a first step you can look for all uses of EvalContext and rename them to InterpretCx

Centril added a commit to Centril/rust that referenced this issue Mar 27, 2019
…erpretcx, r=oli-obk

Renames `EvalContext` to `InterpretCx`

This PR renames `EvalContext` to `InterpretCx` in `src/librustc_mir`.
This PR is related to rust-lang#54395 .
Centril added a commit to Centril/rust that referenced this issue Mar 27, 2019
…erpretcx, r=oli-obk

Renames `EvalContext` to `InterpretCx`

This PR renames `EvalContext` to `InterpretCx` in `src/librustc_mir`.
This PR is related to rust-lang#54395 .
Centril added a commit to Centril/rust that referenced this issue Mar 27, 2019
…erpretcx, r=oli-obk

Renames `EvalContext` to `InterpretCx`

This PR renames `EvalContext` to `InterpretCx` in `src/librustc_mir`.
This PR is related to rust-lang#54395 .
cuviper added a commit to cuviper/rust that referenced this issue Mar 28, 2019
…erpretcx, r=oli-obk

Renames `EvalContext` to `InterpretCx`

This PR renames `EvalContext` to `InterpretCx` in `src/librustc_mir`.
This PR is related to rust-lang#54395 .
@kenta7777
Copy link
Contributor

As the next step, should I rename EvalErrorKind to InterpretErrorKind or make a list for renames?

@eddyb
Copy link
Member

eddyb commented Mar 30, 2019

For errors/results, maybe we can do the io::{Error, Result} thing and have interpret::{Error, Result}?

@Centril
Copy link
Contributor

Centril commented Mar 30, 2019

I think that can turn rather ambiguous and contextual in a large codebase like rustc.

@eddyb
Copy link
Member

eddyb commented Mar 30, 2019

It bothers me how much shorter Miri is than Interpret.
Maybe IError and IResult?

@Centril
Copy link
Contributor

Centril commented Mar 30, 2019

@eddyb InterpError?

@eddyb
Copy link
Member

eddyb commented Mar 30, 2019

I have to say that shorter prefixes are ambiguous, and the only other shortenings I can think of involve removing vowels, i.e. IntrptError. Nevermind, "interp" only has one non-leading vowel...
So MiriError or InterpError it is?

@Centril
Copy link
Contributor

Centril commented Mar 30, 2019

It is.

@kenta7777
Copy link
Contributor

Could I rename EvalErrorKind to InterpError?
similary, EvalResult to InterpResult, EvalSnapshot to InterpSnapshot?

@kenta7777
Copy link
Contributor

kenta7777 commented Apr 1, 2019

This is a list for renames.

  • EvalErrorKind to InterpError
  • EvalResult to InterpResult
  • EvalSnapshot to InterpSnapshot

Centril added a commit to Centril/rust that referenced this issue Apr 2, 2019
…InterpError, r=oli-obk

Renames `EvalErrorKind` to `InterpError`

This PR renames `EvalErrorKind` to `InterpError`.
This is related to rust-lang#54395.
Centril added a commit to Centril/rust that referenced this issue Jun 7, 2019
Rename remaining "Eval" to "Interp"

Renaming done by sed.

r? @oli-obk

Fixes rust-lang#54395.
Centril added a commit to Centril/rust that referenced this issue Jun 7, 2019
Rename remaining "Eval" to "Interp"

Renaming done by sed.

r? @oli-obk

Fixes rust-lang#54395.
bors added a commit that referenced this issue Jun 8, 2019
Rename remaining "Eval" to "Interp"

Renaming done by sed.

r? @oli-obk

Fixes #54395.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants