[PATCH] D44034: Reland "[WebAssembly] More uses of uint8_t for single byte values"

Sam Clegg via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 2 13:40:17 PST 2018


sbc100 added a comment.

In https://reviews.llvm.org/D44034#1025945, @ncw wrote:

> LGTM.
>
> The assert came from me, and it was carefully checking the letter and the spirit of the current spec!
>
> I'm not entirely convinced by the change overall - I personally thought it was good to use SLEB before, rather than uint8_t. There's a note in the official spec which makes it very clear "these are really SLEB values in intent", so changing the implementation to uint8_t seems a bit backwards. Yes it matches the official text, but not the official direction of the spec. We're only going to have to put it back to SLEB sometime in the future.
>
> Oh well, it should be a safe change in any case.


I've been making these changes across the board, both in llvm and in wabt.   I found it rather strange that the byte constants in the spec did not match those in the code.    Also, the spec specificlly says they are single byte values (for now anyway).


Repository:
  rL LLVM

https://reviews.llvm.org/D44034





More information about the llvm-commits mailing list