abrown opened PR #2240 from refactor-ext-mode
to main
:
In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place
ExtMode::calculate
and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used inExtMode::calculate
is correct in all of the places it is used; some of these only handled, e.g.{8 -> 64, 16 -> 64, 32 -> 64}
, and it is unclear to me whether the other extension modes should be matched here.
abrown updated PR #2240 from refactor-ext-mode
to main
:
In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place
ExtMode::calculate
and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used inExtMode::calculate
is correct in all of the places it is used; some of these only handled, e.g.{8 -> 64, 16 -> 64, 32 -> 64}
, and it is unclear to me whether the other extension modes should be matched here.
abrown updated PR #2240 from refactor-ext-mode
to main
:
In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place
ExtMode::calculate
and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used inExtMode::calculate
is correct in all of the places it is used; some of these only handled, e.g.{8 -> 64, 16 -> 64, 32 -> 64}
, and it is unclear to me whether the other extension modes should be matched here.
abrown updated PR #2240 from refactor-ext-mode
to main
:
In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place
ExtMode::calculate
and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used inExtMode::calculate
is correct in all of the places it is used; some of these only handled, e.g.{8 -> 64, 16 -> 64, 32 -> 64}
, and it is unclear to me whether the other extension modes should be matched here.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Shouldn't this be
src_ty.bits() < dst_ty.bits() || ext_mode.is_none()
?
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
calculate
made me think that there would be some complicated calculation here happening here; how about using a plainnew
here? (orinfer
, orfor
which would read nicelyExtMode::for(32, 64)
)
abrown submitted PR Review.
abrown created PR Review Comment:
Yeah, I like
for
or evenfrom
... ugh, choosing names. I'll go withfor
.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
for
is a keyword, so you can't use it as identifier.
abrown submitted PR Review.
abrown created PR Review Comment:
Then I guess
new
orfrom
?
abrown created PR Review Comment:
Yeah, looking at it again I think it should actually be:
(src_ty.bits() < dst_ty.bits() && ext_mode.is_some()) || ext_mode.is_none()
. Either we have the right size bits and we get a properSome(ExtMode)
or we don't and get aNone
--anything else should be an error.
abrown submitted PR Review.
abrown updated PR #2240 from refactor-ext-mode
to main
:
In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place
ExtMode::calculate
and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used inExtMode::calculate
is correct in all of the places it is used; some of these only handled, e.g.{8 -> 64, 16 -> 64, 32 -> 64}
, and it is unclear to me whether the other extension modes should be matched here.
abrown submitted PR Review.
abrown created PR Review Comment:
Went with
new
.
abrown submitted PR Review.
abrown created PR Review Comment:
Fixed.
abrown merged PR #2240.
Last updated: Dec 23 2024 at 12:05 UTC