Commit e68a4be8 authored by Eric Bruneton's avatar Eric Bruneton
Browse files

Add an exception message for unsupported class versions.

parent 63924390
Pipeline #610 passed with stage
in 5 minutes and 45 seconds
......@@ -159,7 +159,8 @@ public class ClassReader {
// Check the class' major_version. This field is after the magic and minor_version fields, which
// use 4 and 2 bytes respectively.
if (checkClassVersion && readShort(classFileOffset + 6) > Opcodes.V10) {
throw new IllegalArgumentException();
throw new IllegalArgumentException(
"Unsupported class file major version " + readShort(classFileOffset + 6));
}
// Create the constant pool arrays. The constant_pool_count field is after the magic,
// minor_version and major_version fields, which use 4, 2 and 2 bytes respectively.
......
  • Given it's a constructor, i would prefer to keep the constructor small (in term of bytecode size) to help inlining (thus escape analysis). Given that throwing an exception is an infrequent path, i think the creation of the exception should be defined in its own method.

    if (checkClassVersion && readShort(classFileOffset + 6) > Opcodes.V10) {`
      throw newIllegalArgumentException();
    }
    ...
    
    private IllegalArgumentException newIllegalArgumentException() {
      return new IllegalArgumentException(
          "Unsupported class file major version " + readShort(classFileOffset + 6));
    }
    Edited by Remi Forax
  • This seems really weird to me. If a separate method is really needed, I would prefer something like

        if (checkClassVersion) {
          checkClassVersion(readShort(classFileOffset + 6)) {
        }

    But I'm not convinced we need a separate method. The inlining argument seems weak given the already large size of this constructor.

  • It's a little weird, but i find this pattern readable, you now that it will throw an exception and do not consume a lot of bytecode.

    Anyway, i agree that the inlining argument is not strong point, so let not create a separate method for that.

    so your proposed change LGTM.

Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment