-
Notifications
You must be signed in to change notification settings - Fork 5.9k
8358772: Template-Framework Library: Primitive Types #25672
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.
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
base: master
Are you sure you want to change the base?
8358772: Template-Framework Library: Primitive Types #25672
Conversation
👋 Welcome back epeter! A progress list of the required criteria for merging this PR into |
@eme64 This change is no longer ready for integration - check the PR body for details. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another great addition to the Template Framework! Thank you for your continued effort, @eme64.
The broad strokes look good to me, but I have some remarks about a few details.
test/hotspot/jtreg/compiler/lib/template_framework/library/CodeGenerationDataNameType.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public boolean isSubtypeOf(DataName.Type other) { | ||
return (other instanceof PrimitiveType pt) && pt.kind == kind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be useful to implement the primitive type subtyping rules from JLS §4.10.1. I can imagine that it might help generating more diverse programs with random variables of random primitive types. That might help with fuzzing IGVN optimizations on ranges?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice idea :)
I'll file a separate RFE for this. It is not a prime feature I need now, but it would be a good extension.
The user can still get exact behavior with exactOf
, but also subtype behavior with subtypeOf
. Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed: JDK-8359335
test/hotspot/jtreg/compiler/lib/template_framework/library/PrimitiveType.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestPrimitiveTypes.java
Outdated
Show resolved
Hide resolved
// Create a new CompileFramework instance. | ||
CompileFramework comp = new CompileFramework(); | ||
|
||
// Add a java source file. | ||
comp.addJavaSourceCode("p.xyz.InnerTest", generate()); | ||
|
||
// Compile the source file. | ||
comp.compile(); | ||
|
||
// p.xyz.InnerTest.main(); | ||
comp.invoke("p.xyz.InnerTest", "main", new Object[] {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I would remove the comments here, since this is not a tutorial about the compile framework and the code is self-explanatory (the methods match the comments pretty well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still an example, so I'll leave it. It doesn't hurt. But I may drop them in the future :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice additions to the library! Lot of minor comments, otherwise, looks good.
test/hotspot/jtreg/compiler/lib/template_framework/library/CodeGenerationDataNameType.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/library/CodeGenerationDataNameType.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/library/CodeGenerationDataNameType.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/library/PrimitiveType.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/library/PrimitiveType.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestPrimitiveTypes.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestPrimitiveTypes.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestPrimitiveTypes.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestPrimitiveTypes.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Hagedorn <[email protected]> Co-authored-by: Manuel Hässig <[email protected]>
Co-authored-by: Christian Hagedorn <[email protected]> Co-authored-by: Manuel Hässig <[email protected]>
@chhagedorn @mhaessig Thanks for the extremely quick reviews and the good suggestions. I addressed them all :) |
|
||
/** | ||
* List of all {@link PrimitiveType}s. | ||
*/ | ||
public static final List<PrimitiveType> PRIMITIVE_TYPES = List.of( | ||
static final List<PrimitiveType> PRIMITIVE_TYPES = List.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static final
can also be removed for constants :-) Otherwise, looks good, thanks for the update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing our comments so quickly. Looks good!
I would like to add primitive type support to the template framework library.
In follow-up work, we will use these types in random expression generation - but they can also already be useful on their own now.
I encountered an issue with some methods that return
Token
from theTemplateFramework
, such asHook.insert
andaddDataName
. SinceToken
was package private, this class could not be used in some places where automatic type inference is required. I now refactored the code, so that theToken
is just an empty interface, and all the methods are moved to a separate classTokenParser
.Original experiments from here: #23418
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25672/head:pull/25672
$ git checkout pull/25672
Update a local copy of the PR:
$ git checkout pull/25672
$ git pull https://git.openjdk.org/jdk.git pull/25672/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25672
View PR using the GUI difftool:
$ git pr show -t 25672
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25672.diff
Using Webrev
Link to Webrev Comment