Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 15645 Build 15645: arc lint + arc unit
Event Timeline
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp | ||
---|---|---|
97 | There was no assert before and this is no more dangerous so I wouldn't add one as part of this change. If you must at a new assert the text should maybe say "operand signaure is not a valid wasm value type", and the check should be a range change of valid wasm types. But I would just drop this assert. |
I think this change might fix the same issue: D44037
But not reason not to land both.
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).
There was no assert before and this is no more dangerous so I wouldn't add one as part of this change.
If you must at a new assert the text should maybe say "operand signaure is not a valid wasm value type", and the check should be a range change of valid wasm types.
But I would just drop this assert.