Introduce Le<T> and Ne<T> newtypes for little- and native-endian integers#13193
Introduce Le<T> and Ne<T> newtypes for little- and native-endian integers#13193fitzgen wants to merge 1 commit intobytecodealliance:mainfrom
Le<T> and Ne<T> newtypes for little- and native-endian integers#13193Conversation
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks! The overall goal seems pretty reasonable here -- some thoughts below.
|
|
||
| impl<T> $name<T> { | ||
| #[inline] | ||
| const fn is_little() -> bool { |
There was a problem hiding this comment.
FWIW the is_little name was a bit confusing for me at first because is_little = false sounds (naively) like "big endian"; it's not, it's "native endian", which could be actually big or little depending on host.
Maybe is_explicit_little?
There was a problem hiding this comment.
(After writing this comment, see more general thoughts below on the confusion)
|
|
||
| use core::{fmt, num::NonZero}; | ||
|
|
||
| macro_rules! define_endian_wrapper_types { |
There was a problem hiding this comment.
Thanks for the newtype-correctness efforts here!
I'm finding the naming/conceptual definition a little confusing, to be honest, because (in my head) the integer types themselves have an abstract domain (0..2^width) and their byte representations have endianness. This aligns with e.g. the builtin uNN::to_le_bytes() -- the LE representation is a sequence of bytes, not a uNN.
So to that end, what does it mean when we have a Le(1234)? Does that mean the "actual value" is 1234 but the value is stored in memory as either 1234 (little-endian host) or bswap(1234) (big-endian host)? Or vice-versa, the value is stored in memory however the host does and we interpret it as LE ("1234-native-endian, interpreted as LE")? (EDIT: I guess these two views are actually equivalent in results, because on a LE host, Le and Ne both mean "identity", and on a BE host, Le means "byteswapped"; but that still leaves a confusing definitional question unanswered)
I think I might have an easier time if we rename things a bit -- or failing that (since these types are very pervasive), document better. Suggestions:
ExplicitLittleEndian(n)andHostOrdered(n)- store
[u8; N]in the newtypes and make these traits wrappers around{from,to}_{le,ne}_bytes - something else I can't think of right now
I kind of favor the second, assuming that LLVM can see through it and not pessimize -- Godbolt example to demonstrate that I think it should: link
| definition.write_gc_ref(&mut store, new); | ||
| } else { | ||
| definition.init_gc_ref(&mut store, new); | ||
| } |
There was a problem hiding this comment.
These seem like good fixes but possibly unrelated to endianness?
| #[inline] | ||
| fn needs_gc_before_next_growth(&self) -> bool { | ||
| false | ||
| } |
| // any growth failures here. | ||
| let _ = store.grow_gc_heap(limiter.as_mut(), bytes_needed).await; | ||
| let _ = store | ||
| .grow_gc_heap(limiter.as_mut(), bytes_needed, asyncness) |
There was a problem hiding this comment.
asyncness change unrelated to endianness?
|
|
||
| (call $gc) | ||
| (drop (array.new $arr (i32.const 0) (i32.const 5))) | ||
| ;; (drop (array.new $arr (i32.const 0) (i32.const 5))) |
There was a problem hiding this comment.
Lots of commented-out lines in testsuite lines here and below -- revert?
github-actions
Bot
added
wasmtime:api
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
2 participants
Split out from #13107 while debugging s390x issues.