codeconsole commented on PR #3024:
URL: https://github.com/apache/groovy/pull/3024#issuecomment-6400630045
Pushed 9d2a811ee9. Replies in the order of the review:
1. Kept, documented and tested in 6190c30aaf. The asymmetry is the
runtime's. With indy, the cast to `Boolean` returns `null` as is
(`CastSelector.handleBoolean`) but calls `asBoolean` through the meta class on
any other value, so inside a category that overrides `asBoolean(Boolean)`,
`$INIT` passes `false` for `true`. A direct `true` would diverge there; a
direct `null` cannot. A `boolean` parameter unboxes without `asBoolean`, so
`true` and `false` go direct there. `Boolean.TRUE` is a property expression,
not a literal, so it keeps `$INIT`.
`testBooleanArgumentsFollowTheCastOfTheRuntime` checks the path for each
combination, and `testBooleanArgumentsUnderACategoryThatOverridesAsBoolean` the
values; allowing `true` for `Boolean` fails both, rejecting `null` fails the
first.
2. Fixed in 083bd6b24f, in both places: `completeEnum` had the same cast, so
a guard in `canInitDirectly` alone would only have moved the
`ClassCastException` (master throws it there too). A non-list initial value is
now the only argument, as `EnumHelper.addEnumConstant` treats it, and then
follows the usual rules. The `testOnlyArgumentOutsideAList*` tests use a parser
plugin that leaves the argument unwrapped; they fail without the change.
3. Changed in 001ced2e4d: a varargs constructor now disables direct init
only where it can win. `calculateParameterDistance` adds at least `1 << 44` to
a varargs call, which outweighs the distance of the matching-arity constructor,
except when the varargs constructor has the call's arity and the last argument
is `null` (no penalty: master picks `E(String, Object...)` over `E(String,
Integer)` for `('two', null)`). That case, and constants that match only as
varargs, keep `$INIT`, with a comment on the `return null`. A 912-enum varargs
matrix (dynamic and `@CompileStatic`, indy and classic) matches master value
for value.
`testVarargsConstructorBesideMatchingConstructorCallsConstructorDirectly`,
`testVarargsConstructorForAnyArgumentsBesideMatchingConstructorCallsConstructorDirectly`,
`testNullLastArgumentForAVarargsConstructorOfTheSameArityUsesInitHelper`,
`EnumTest.testLiteralArgumentsBesideAVarargsConstructor`.
4. Fixed in e50ac46974: the class generator writes the call through
`visit(GroovyCodeVisitor)`, which passes itself to `write(mv, generator)`, and
no field holds it. `visit(MethodVisitor)` still writes a constant without
arguments and throws a `GroovyBugError` instead of an NPE for one with
arguments, since only the generator can load those (emitting the `$INIT` call
needs it too). `testOnlyTheClassGeneratorWritesTheArgumentsOfAConstant`.
5. Fixed in e50ac46974: any other visitor visits `initCall` again. Such
visitors exist: the AST browser and `AstNodeToScriptAdapter` run after the
class generator at the Class Generation phase. They showed
`/*BytecodeExpression*/` at 48a40f5b6b and show `E.$INIT('ONE', 0, 'one')`
again, as on master.
`testOtherVisitorsSeeTheInitCallsThatTheDirectCallsReplace`.
6. Added to the Javadoc of `substitute` in 0719570757: transforms that
rewrite `$INIT` decide the path, and so whether the enum needs constructor
reflection, in both directions
(`testArgumentAddedByATransformToEveryConstantCallsConstructorDirectly`,
`testInitCallReplacedByATransformKeepsTheEnumOnTheEmittedCalls`).
7. Spelled out in the class Javadoc (0719570757) and the PR description:
enums whose constants all have literal arguments, which always went through
`$INIT` before, are now created out of a meta class's sight too. Their opt-out
is one non-literal argument, such as `'one'.toString()` for `'one'`
(`testNonLiteralArgumentKeepsConstantCreationOpenToMetaClassInterception`).
8. `box()` returns at once for a reference type (OperandStack.java:233).
`testClassLiteralArgumentsCallConstructorDirectly` (6997620fe3) pins `LDC
String.class`, `LDC Integer.class` and `GETSTATIC Integer.TYPE`, each followed
directly by `INVOKESPECIAL`.
Test gaps:
- `Boolean`/`null`: item 1 (6190c30aaf).
- `ListExpression` guard: item 2 (083bd6b24f).
- Class literal: item 8 (6997620fe3).
- `Character`/`char`: added in 6997620fe3. Groovy has no char literal, so a
transform adds a `Character` constant. It goes direct to `char` and
`Character`, and keeps `$INIT` for `int`, where the runtime finds no
constructor rather than widening (`testCharacterConstantArguments`,
`EnumTest.testCharacterConstantArgumentsAddedByATransform`).
- Static compiler's `$INIT` form:
`testInitCallsInTheFormOfTheStaticCompilerCallConstructorDirectly` (6997620fe3)
asserts the `MethodCallExpression` form just before class generation and the
direct bytecode;
`testTransformsSeeTheInitCallsOfConstantsThatAreCreatedDirectly` asserts the
`StaticMethodCallExpression` form of a dynamic enum. Disabling either branch of
`initArguments` fails them.
Nits:
- `transformExpression`: no change.
- Named in 9d2a811ee9:
`org.codehaus.groovy.classgen.EnumConstantInit.constantAssignments`, like the
`ClosureWriter` keys.
- `setSourcePosition(initCall)`: still in place.
- By design; stated in the Javadoc of `substitute` (0719570757). With item
5, visitors that run after the class generator see the `$INIT` calls too.
Without the change, 26 tests fail: 24 of the 50 new tests and the 2
rewritten ones. The matrices in the description were re-run on this head with
indy and with classic call sites; the values and exceptions match master apart
from closure identity hashes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
