8355652: Parse ClassFileFormatVersion from ClassFileVersion

[gustavosimon]

8355652: add new method to return ClassFileFormatVersion from ClassFileVersion.

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8355652: Parse ClassFileFormatVersion from ClassFileVersion (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26406/head:pull/26406
$ git checkout pull/26406

Update a local copy of the PR:
$ git checkout pull/26406
$ git pull https://git.openjdk.org/jdk.git pull/26406/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26406

View PR using the GUI difftool:
$ git pr show -t 26406

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26406.diff

Using Webrev

Link to Webrev Comment

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request matching fixVersion 26 to be approved (needs to be created)

Issue

• JDK-8355652: Parse ClassFileFormatVersion from ClassFileVersion (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26406/head:pull/26406
$ git checkout pull/26406

Update a local copy of the PR:
$ git checkout pull/26406
$ git pull https://git.openjdk.org/jdk.git pull/26406/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26406

View PR using the GUI difftool:
$ git pr show -t 26406

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26406.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back gustavosimon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@gustavosimon The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (d4ff0b6d)
• 00: Full (ab67e498)

[gustavosimon]

@liach I know that I'm commenting on a weekend but it's to you see tomorrow.

I'm commiting a 1.0 version for this implementation - to get your feedback about

I think we should create some unit tests for this API and classes involved. Where do you think that is the most appropriate location in the project to do it?

[liach]

I think we should create some unit tests for this API and classes involved. Where do you think that is the most appropriate location in the project to do it?

Unit tests for java.lang.classfile reside in test/jdk/jdk/classfile. They can be run with make test TEST=jdk/jdk/classfile.

[gustavosimon]

I think we should create some unit tests for this API and classes involved. Where do you think that is the most appropriate location in the project to do it?

Unit tests for java.lang.classfile reside in test/jdk/jdk/classfile. They can be run with make test TEST=jdk/jdk/classfile.

Great! Thanks!

[liach]

You should check the minor version to be 0 for major versions 54 and later.

[gustavosimon]

Do you mean something like this?

@Override
    public Optional<ClassFileFormatVersion> formatVersion() {
        if (majorVersion < 54 || minorVersion != 0) {
            return Optional.empty();
        }
        try {
            return Optional.of(ClassFileFormatVersion.fromMajor(majorVersion));
        } catch (IllegalArgumentException e) {
            return Optional.empty();
        }
    }            

[liach]

No, this logic is wrong. It's more like the check in

jdk/src/java.base/share/classes/jdk/internal/misc/VM.java

Lines 174 to 180 in 441dbde

	public static boolean isSupportedClassFileVersion(int major, int minor) {
	if (major < ClassFile.JAVA_1_VERSION || major > ClassFile.latestMajorVersion()) return false;
	// for major version is between 45 and 55 inclusive, the minor version may be any value
	if (major < ClassFile.JAVA_12_VERSION) return true;
	// otherwise, the minor version must be 0 or 65535
	return minor == 0 || (minor == ClassFile.PREVIEW_MINOR_VERSION && major == ClassFile.latestMajorVersion());
	}

 public Optional<ClassFileFormatVersion> formatVersion() { 
     if (majorVersion < ClassFile.JAVA_1_VERSION || majorVersion > ClassFile.latestMajorVersion()) return Optional.empty(); 
     // for major version is between 45 and 55 inclusive, the minor version may be any value 
     if (majorVersion >= ClassFile.JAVA_12_VERSION && minorVersion != 0) return Optional.empty(); 
     // otherwise, only minor version 0 is a defined, persistent format
     return Optional.of(ClassFileFormatVersion.fromMajor(majorVersion)); 
 } 

[gustavosimon]

Got it. Could you have this hook in a private class method or a static public method?
Maybe it can be reused in another points?

[liach]

Maybe it can be reused in another points?

ClassFileFormatVersion and Optional are more user-oriented classes; internal code don't really ever use these two types, so I don't think you need to extract this logic specifically.

[gustavosimon]

I mean about the rule the version - similar to isSupportedClassFileVersion method

[liach]

I still don't think so. I intentionally proposed the original RFE so we have one place where we can consolidate this handling, and that place should be here instead of some other place.

[gustavosimon]

Great! I'll do this way

[myankelev]

Nitpick: copyright year :)

[gustavosimon]

@myankelev I updated the copyright year in ClassFileVersionImpl

[liach]

/csr

[openjdk]

@liach has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@gustavosimon please create a CSR request for issue JDK-8355652 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.