On Fri, 27 Feb 2026 23:43:51 GMT, Joe Darcy <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> More tests and tidying
>
> make/jdk/src/classes/build/tools/valhalla/valuetypes/GenValueClasses.java
> line 76:
>
>> 74: * we must generate sources for abstract classes, we only process one of
>> them.
>> 75: * <ul>
>> 76: * <li>{@code @jdk.internal.ValueBased} appears on concrete value
>> classes.
>
> I would have expected to the processor to process (and likely claim) both
> annotations. Can you say more here?
I'm happy to do that, but since we need to convert all value classes (abstract
and concrete) the only one we *need* is @jdk.internal.MigratedValueClass.
Claiming the annotation(s) is something I thought about, but someone might
decide they want to use them, say, for documentation or other tests, so I
decided against it.
> make/jdk/src/classes/build/tools/valhalla/valuetypes/GenValueClasses.java
> line 92:
>
>> 90:
>> 91: @Override
>> 92: public synchronized void init(ProcessingEnvironment processingEnv) {
>
> Was there a reason using the Filer API was ruled out?
After talking with Jan, this seemed the better option.
What would the Filer API have simplified here?
> make/jdk/src/classes/build/tools/valhalla/valuetypes/GenValueClasses.java
> line 141:
>
>> 139: if (!e.getKind().isClass()) {
>> 140: throw new IllegalStateException(
>> 141: "Unexpected element kind (" + e.getKind() + ")
>> for element: " + e);
>
> Was using the Messager to emit an error here considered and rejected?
I'm not actually familiar with the Messager API, so I'll have a look at that.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2180#discussion_r2872199289
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2180#discussion_r2872221558
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2180#discussion_r2872223695
