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

"Standardize" name for variables of type InterpCx #3958

Open
Mandragorian opened this issue Oct 10, 2024 · 1 comment
Open

"Standardize" name for variables of type InterpCx #3958

Mandragorian opened this issue Oct 10, 2024 · 1 comment
Labels
A-style Area: coding style C-cleanup Category: cleaning up our code E-good-first-issue A good way to start contributing, mentoring is available

Comments

@Mandragorian
Copy link
Contributor

When I made my first contribution to the main miri code base, I was for a brief moment (slightly) confused about the variables this and ecx, mostly used to store the result of eval_context_{ref,mut}. It would be nice if the name was consistent.

I see that this is most commonly used:

$ grep -r "this: " | wc -l
62
$ grep -r "let this = " | wc -l
341
$ grep -r "ecx: " | wc -l
107
$ grep -r "let ecx = " | wc -l
8

However, sometimes this is used to refer to other values, for example:

borrow_tracker/tree_borrows/tree.rs:        this: &mut TreeVisitor<'_>,

So personally I would propose using ecx to store the interpreter context.

Obviously this is not really important, and adds no value to the users, but if you agree that this might improve the code base, I am glad to send a PR.

@RalfJung
Copy link
Member

We generally use this when we'd like to use self but cannot. So that's for all the methods in a EvalContextExt impl. You're not confused that self has many different types throughout the codebsae, so you shouldn't expect that from this either.

However. some of the ecx should indeed be this, e.g. in src/alloc_addresses/mod.rs. Basically all cases of let ecx = self.eval_context_ref/let ecx = self.eval_context_mut should be changed to this. But the ecx: ... should stay the way they are, IMO.

@RalfJung RalfJung added E-good-first-issue A good way to start contributing, mentoring is available C-cleanup Category: cleaning up our code A-style Area: coding style labels Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-style Area: coding style C-cleanup Category: cleaning up our code E-good-first-issue A good way to start contributing, mentoring is available
Projects
None yet
Development

No branches or pull requests

2 participants