Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add test for VarHandle MethodHandleInfo #4541

Merged
merged 1 commit into from
Apr 4, 2019
Merged

Add test for VarHandle MethodHandleInfo #4541

merged 1 commit into from
Apr 4, 2019

Conversation

pdbain-ibm
Copy link
Contributor

Depends on
#4458

Signed-off-by: Peter Bain peter_bain@ca.ibm.com

DanHeidinga
DanHeidinga reviewed Feb 12, 2019
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@llxia Can you or a delegate review this as well?

@pdbain-ibm
Copy link
Contributor Author

@DanHeidinga and @llxia
Updated per your comments and re-tested.

In re efficiency, this test runs in less than 2 seconds, so I favoured clarity over speed.
Thanks for the input on functional vs. imperative: I will keep that in mind in future.

llxia
llxia approved these changes Feb 14, 2019
View reviewed changes
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pdbain-ibm
Copy link
Contributor Author

Can you use a unique name for each of the MTs rather than adding this extra scoping? It will be more clear for readers what's happening

Done.

nitpick - missing space between am,vh

Fixed.

@pdbain-ibm pdbain-ibm force-pushed the varhandle_test branch 2 times, most recently from e7abd57 to 812050d Compare February 14, 2019 19:32
DanHeidinga
DanHeidinga reviewed Feb 14, 2019
View reviewed changes
test/functional/Java11andUp/src/org/openj9/test/varhandle/TestVarHandleInfo.java Outdated
protected static Logger logger = Logger.getLogger(TestVarHandleInfo.class);
private static final String TEST_VAR_HANDLE_INFO = "Lorg/openj9/test/varhandle/TestVarHandleInfo;"; //$NON-NLS-1$
private static final String INTEGER = "Ljava/lang/Integer;"; //$NON-NLS-1$
private static final int MODIFIER_SYNTHETIC = 0x01000; /* get this from dumping java/lang/reflect/Modifier */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

/* get this from dumping java/lang/reflect/Modifier */

It's also in the VM spec :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DanHeidinga
I need a newer copy of the spec -:).
Code updated with chapter and verse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see the /* get this from dumping java/lang/reflect/Modifier */ comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with reference to VM spec.

@pdbain-ibm
Copy link
Contributor Author

Updated per reviewers' comments.

@pdbain-ibm
Copy link
Contributor Author

@llxia or @DanHeidinga do you have any further comments?

DanHeidinga
DanHeidinga reviewed Apr 2, 2019
View reviewed changes
test/functional/Java11andUp/src/org/openj9/test/varhandle/TestVarHandleInfo.java Outdated
protected static Logger logger = Logger.getLogger(TestVarHandleInfo.class);
private static final String TEST_VAR_HANDLE_INFO = "Lorg/openj9/test/varhandle/TestVarHandleInfo;"; //$NON-NLS-1$
private static final String INTEGER = "Ljava/lang/Integer;"; //$NON-NLS-1$
private static final int MODIFIER_SYNTHETIC = 0x01000; /* get this from dumping java/lang/reflect/Modifier */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see the /* get this from dumping java/lang/reflect/Modifier */ comment

@pdbain-ibm
Add test for VarHandle MethodHandleInfo
c6a30b1 
Test for eclipse-openj9#3180

This depends on eclipse-openj9#4458

[ci skip]

Signed-off-by: Peter Bain <peter_bain@ca.ibm.com>
@pdbain-ibm
Copy link
Contributor Author

Updated per reviewer's comments. I also trimmed out some redundant code and cleaned up formatting. Tested on MacOS and pushed.

DanHeidinga
DanHeidinga approved these changes Apr 2, 2019
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk11

@DanHeidinga
Copy link
Member

Jenkins test sanity osx jdk12

@DanHeidinga DanHeidinga self-assigned this Apr 2, 2019
@pdbain-ibm
Copy link
Contributor Author

@DanHeidinga could you restart the sanity build? It appears there was a machine problem.

@DanHeidinga
Copy link
Member

Jenkins test sanity osx jdk12

@DanHeidinga DanHeidinga merged commit 1fadd1b into eclipse-openj9:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdbain pdbain pdbain left review comments

@DanHeidinga DanHeidinga DanHeidinga approved these changes

@llxia llxia llxia approved these changes

Assignees

@DanHeidinga DanHeidinga

Labels
comp:test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pdbain-ibm @DanHeidinga @pdbain @llxia @pshipton