Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Simplify and rename Stackmap::from_vec #1032

Merged
merged 5 commits into from
Sep 18, 2019
Merged

Simplify and rename Stackmap::from_vec #1032

merged 5 commits into from
Sep 18, 2019

Conversation

AnthonyMikh
Copy link
Contributor

Functionality is preserved, all tests pass.

It is unclear if this method really should take &Vec<bool> rather than &[bool]. Should I make this change as well?

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 14, 2019

Should I make this change as well?

I think so.

@vitiral
Copy link

vitiral commented Sep 16, 2019

This is really good code! I like the change from float -> int operations and the use of chunks for the vector. I bet this is more performant too, since it avoids bounds checks.

The only thing is... the method name from_vec is wrong now that it's from a slice instead of a vec 😆

@AnthonyMikh
Copy link
Contributor Author

@vitiral what name do you propose then? I think about from_bools/from_flags. It shouldn't be a hard change since from_vec is used in this single file only.

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 16, 2019

from_slice?

@AnthonyMikh
Copy link
Contributor Author

@bjorn3 IMHO, from_slice is not descriptive enough. The fact that argument is slice is already known from types. I suppose that the name should carry a bit more information about the meaning of argument.

@vitiral
Copy link

vitiral commented Sep 16, 2019

it is a constructor. If there were overloading you would just use from for everything and then overloading would take care of the type. _slice should precisely communicate the type, because it is basically just a stand-in for overloading!

@AnthonyMikh AnthonyMikh changed the title Simplify Stackmap::from_vec Simplify and rename Stackmap::from_vec Sep 16, 2019
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice simplification.

cranelift-codegen/src/binemit/stackmap.rs Outdated Show resolved Hide resolved
cranelift-codegen/src/binemit/stackmap.rs Outdated Show resolved Hide resolved
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, thanks a lot! It's a sweet simplification.

@bnjbvr
Copy link
Member

bnjbvr commented Sep 18, 2019

(I'll squash before merging; in general we try to keep commits "atomic" (= do one logical thing, passes test in isolation) so as to keep git bisection trivial. No worries about not doing so this time, just wanted to let you know for next time :))

@bnjbvr bnjbvr merged commit 5045a97 into bytecodealliance:master Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants