bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
Makes sense
bjorn3 created PR Review Comment:
ConstantData
is stored in little endian format.ConstantDataU128
converts it to a native endianu128
, so no swap should be necessary.
bjorn3 submitted PR Review.
bnjbvr requested fitzgen, and abrown for a review on PR #1231.
fitzgen submitted PR Review.
fitzgen submitted PR Review.
fitzgen created PR Review Comment:
We shouldn't panic on parse errors, we should return a fallible
ParseResult
like other parsing functions do.Additionally, it seems a little strange that we have this parsing code in the
cranelift-filetests
crate, when AFAICT all the other parsing code is in thecranelift-reader
crate. Unless there is a good reason not to, we should match existing conventions and keep the parsing code insidecranelift-reader
.
fitzgen created PR Review Comment:
It isn't clear to me what the resolution is here, if any.
fitzgen created PR Review Comment:
Was this still going to happen?
fitzgen created PR Review Comment:
I believe that moving that parsing code into this
cranelift-reader
crate would let us avoid making all these methods public as well.
fitzgen created PR Review Comment:
Instead of making an ad-hoc
try_into
method, we should follow convention and implement theTryFrom
trait, which gives us atry_into
via the blanket implTryInto<T> for U
whereU: TryFrom<T>
. For details, see: https://doc.rust-lang.org/stable/std/convert/trait.TryInto.html and https://doc.rust-lang.org/stable/std/convert/trait.TryFrom.htmlIf I'm reading this code right, it would be something like this:
struct $name(ConstantData); impl TryFrom<$name> for $type { // ... }But this also makes me wonder why we don't implement
TryFrom<ConstantData> for $type
directly instead of creating all these temporaryConstantData$Type
types. Is that something that you explored at all? I think it would be the best course to take, unless it doesn't work for some reason or I am overlooking something else.
krk closed without merge PR #1231.
Last updated: Dec 23 2024 at 13:07 UTC