Fix potential bug IntelliJ warnings - #126
Conversation
| // Intentionally get the result as 32 bit `int` (despite it being converted to `long` when | ||
| // pushed to the stack) | ||
| //noinspection IntegerMultiplicationImplicitCastToLong | ||
| stack.push(v << c); |
There was a problem hiding this comment.
IntelliJ was warning that because v and c are int, the result is an int as well and could overflow before it is implicitly converted to long when pushed on the stack.
I assume that is intended here though since this is an operation for i32? Not completely sure though.
ddd98df to
7842962
Compare
andreaTP
left a comment
andreaTP
left a comment
There was a problem hiding this comment.
Looks like good improvements!
I rebased on latest main and found a couple of minor things, added 2 commits on top for you to review @Marcono1234
Marcono1234
left a comment
Marcono1234
left a comment
There was a problem hiding this comment.
Thanks for your additional changes (and for fixing an issue I had made)! Sorry for the late follow-up.
I noticed one more thing (see comment below); should that be adjusted as well?
| private static void I32_MUL(MStack stack) { | ||
| var a = stack.pop(); | ||
| var b = stack.pop(); | ||
| stack.push(a * b); | ||
| } |
There was a problem hiding this comment.
Maybe I32_MUL here is missing (int) casts for the stack operands? All other I32_... methods seem to have that.
The previous commit changed parse(wasmBytes) to parse(() -> new ByteArrayInputStream(wasmBytes)), switching from the static method (which ignores builder config) to the instance method (which applies includeSectionId(CODE)). Parsing only the CODE section causes validation to fail with "function and code section have inconsistent lengths". Use the static Parser.parse(wasmBytes) directly since the builder config was never actually applied and the method needs multiple sections (CODE, FUNCTION, IMPORT).
The previous commit simplified lt(float, float) to "return a <= b" instead of "return a < b", making it identical to le (less-than-or-equal).
0e30301 to
b5a2403
Compare
b5a2403 to
dd837cf
Compare
2 participants
Based on #124; please only consider the commits exclusive to this PR here, currently ddd98df
(in the GitHub UI on the "Files changed" tab you can click "All commits" on the top left to select specific commits)
I had a look at some of the warnings IntelliJ reported for the project, and it seems some of them might be legitimate bugs. Have added comments below. Please let me know what you think.
I mainly wanted to highlight these potential issues; please let me know if I should split the PR or if you want to use it as reference and fix the issues yourself.