On Thu, 9 Dec 2021 11:50:50 GMT, Сергей Цыпанов <d...@openjdk.java.net> wrote:
> `Executable.getParameterTypes()` creates a copy of underlying array which is
> redundant in trusted code when we are sure the action is read-only.
Changes requested by redestad (Reviewer).
src/java.base/share/classes/java/lang/reflect/Constructor.java line 374:
> 372: sb.append('(');
> 373: StringJoiner sj = new StringJoiner(",");
> 374: for (Class<?> parameterType : getSharedParameterTypes()) {
Good.
src/java.base/share/classes/java/lang/reflect/Executable.java line 313:
> 311: // getParameterTypes().
> 312: if (!genericInfo) {
> 313: return getSharedParameterTypes();
Since this returns the trusted array it delegates responsibility to the callers
of `getAllGenericParameterTypes` to not mutate or further expose/leak the
parameter type array. This needs to at least be called out in the method
specification. The comment here needs to be updated, as well. Is the added
fragility in this case worth the performance win?
src/java.base/share/classes/java/lang/reflect/Executable.java line 317:
> 315: final boolean realParamData = hasRealParameterData();
> 316: final Type[] genericParamTypes = getGenericParameterTypes();
> 317: final Type[] nonGenericParamTypes =
> getSharedParameterTypes();
(If removing these allocations is important, I note that
`ConstructorRepository.getGenericParameterTypes()` lacks an equivalent to
`getSharedParameterType` for trusted callers)
Similarly here I'm not sure the win from avoiding the clone is worth delegating
the additional responsibility. You could still do this here if you add
`.clone()` after `nonGenericParamTypes` on L344
src/java.base/share/classes/java/lang/reflect/Method.java line 434:
> 432: String toShortSignature() {
> 433: StringJoiner sj = new StringJoiner(",", getName() + "(", ")");
> 434: for (Class<?> parameterType : getSharedParameterTypes()) {
Good.
-------------
PR: https://git.openjdk.java.net/jdk/pull/6782
