daniellansun commented on PR #2822:
URL: https://github.com/apache/groovy/pull/2822#issuecomment-5377692106
> > Verdict: sound and low-risk; I'd merge.
> > ### Minor nits (none blocking)
> >
> > * The `n == 0` branch in `Selector.sameClassesGuard` is unreachable from
its only call site (`anyMatch` on an empty `pt` is false, and a receiver is
always present). Harmless defensive code, and likely part of codecov's 6
uncovered lines.
> > * Chained `.bindTo().bindTo()...` could be a single
`MethodHandles.insertArguments(h, 0, c0, …, cN)` — marginally more idiomatic
and one adapter instead of four at arity 4; the JIT result is presumably the
same given the measurements.
> > * The `SameClassesGuardMhBench` javadoc says it "compiles only on
GROOVY-12284+" — it actually compiles anywhere (the overloads are resolved via
runtime `findStatic`) and would fail at `@Setup` on a pre-12284 tree. Cosmetic
doc inaccuracy.
> > * The two JMH benches and README section ride along in the PR; that's
consistent with how the ScopedInvalidationBench work landed, and keeping
`dynamic_arity4`/`dynamic_arity5` under GC profiling in the regression rotation
(as the report suggests) is worth doing so a regression back to a universal
collector can't land silently.
Thanks, Paul. All four nits addressed:
- **`n == 0`:** dropped the constant-`true` branch.
Codecov’s missing line there should go with it.
- **`insertArguments`:** done; same helper as above.
- **MH bench javadoc:** it does compile against any tree that has the
public overloads; `@Setup` fails with `NoSuchMethodException` before
GROOVY-12284. The javadoc now says that.
- **Benches:** kept, including `dynamic_arity4` / `dynamic_arity5` for
GC profiling so a return to a universal collector cannot land quietly.
--
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]
