Conversation
giltho
left a comment
There was a problem hiding this comment.
Some nitpicks but overall looks good!
(Reviewing in the Paris metro is not the easiest lmao)
| ok | ||
| (mk_concrete ~size:(ptr_size * 2) ~align:ptr_size | ||
| ~fields:(Array (BV.usizei ptr_size)) | ||
| ()) |
There was a problem hiding this comment.
Btw iirc there's an explicit comment in the core lib saying that external users should never rely on the layout of fat pointers. So maybe one day we should make access raw or opaque depending on access within core or outside.
There was a problem hiding this comment.
hmm this could be interesting yes ! i remember @Nadrieril mentioning that slice layouts or their ABI might eventually become guaranteed, so it would be nice to figure out progress on that (pinging because I cannot find a source anywhere:P )
There was a problem hiding this comment.
The RFC is here: rust-lang/rfcs#3775. Looks stalled for now
There was a problem hiding this comment.
ahhh nice thank you !
|
(You need to promote files) |
Encoder.rust_to_cvals/Encoder.rust_of_cvalswhere both poorly named functions and badly implemented; in particular they relied very extensively on the type of the values, and handling layouts manually, rather than trusting the layout of the type.This PR fixes it! I renamed these functions to
encodeanddecoderespectively, and factored out a lot of code and simplified it. The core idea is:Primitivelayout is written as-is; we don't even check the typeArrayorArbitrarylayout has an iterable set fields;iter_fieldsreturns an iterator over these fields and their offsetEnumlayout needs special treatment to figure out what variant we're reading/writing; however once we know the variant it's straightforward, as above.There is relatively little additional effort needed! I also separated the validity checks done for references into its own function,
check_valid(this comes with the advantage that we can more easily disable validity checks, if we want to one day).This is the second part of #127
Additional minor changes:
IterBinop.Cmpfor pointers, as it's never used