aot suggested fixes #574
aot suggested fixes #574
Conversation
| @@ -367,13 +367,13 @@ private byte[] compileClass(String className, FunctionSection functions) { | |||
| var classWriter = new ClassWriter(ClassWriter.COMPUTE_FRAMES); | |||
| classWriter.visit( | |||
| Opcodes.V11, | |||
| Opcodes.ACC_PUBLIC | Opcodes.ACC_FINAL, | |||
| Opcodes.ACC_PUBLIC | Opcodes.ACC_FINAL | Opcodes.ACC_SUPER, | |||
There was a problem hiding this comment.
Apparently, compatibility with project Leyden.
There was a problem hiding this comment.
Not Leyden, Valhalla. In Valhalla, if you have a class without ACC_SUPER (renamed ACC_IDENTITY), you are creating a value class. See https://bugs.openjdk.org/browse/JDK-8317278
There was a problem hiding this comment.
Many thanks for the explanation!
There was a problem hiding this comment.
I'm guessing this doesn't matter was we're currently generating V11 classes, but good to fix for the future!
| internalClassName, | ||
| null, | ||
| getInternalName(Object.class), | ||
| new String[] {getInternalName(Machine.class)}); | ||
|
|
||
| classWriter.visitSource("wasm", "wasm"); | ||
| classWriter.visitSource(null, null); |
There was a problem hiding this comment.
Better to leave those fields empty than using dummy values.
There was a problem hiding this comment.
The second argument seems wrong for sure. The first one causes stack traces to include wasm, which seems useful, but I don't know if there are downsides. If @forax says we should remove it, then let's remove it!
There was a problem hiding this comment.
As electrum said, for me, it should be
classWriter.visitSource("wasm", null);so you get "wasm" printed in the stacktraces.
There was a problem hiding this comment.
Ok, I have nothing against, my reasoning was:
"better to not provide anything than a wrong fileName that the IDE will try to resolve".
But the IDE is still broken in either cases, so I just opened #576 to track it as a separate issue.
Thanks everyone for the feedback, much appreciated!
| internalClassName, | ||
| null, | ||
| getInternalName(Object.class), | ||
| new String[] {getInternalName(Machine.class)}); | ||
|
|
||
| classWriter.visitSource("wasm", "wasm"); | ||
| classWriter.visitSource(null, null); |
There was a problem hiding this comment.
We can simply remove this call, as calling with null won't do anything.
andreaTP
force-pushed
the
forax-fixes2
branch
3 times, most recently
from
752fe05 to
c4e2c87
Compare
andreaTP
force-pushed
the
forax-fixes2
branch
from
c4e2c87 to
2ca4759
Compare
At Devoxx I had the pleasure to meet @forax , he is one of the maintainer of the ASM library we are using in the AOT compiler and suggested those fixes.