On Tue, 2 Apr 2024 12:50:19 GMT, ExE Boss <d...@openjdk.org> wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing tests.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
> line 1415:
>
>> 1413:
>> canonicalConstructorTypes,
>> 1414: List.nil());
>> 1415: createNew.constructor = init;
>
> Maybe instead of hardcoding the constructor that is canonical at compile time
> (in case new components get added), it might be better to go through some
> sort of `indy` callsite like (at least when not in the same compilation unit):
>
> DynamicCallSiteDesc[java.lang.runtime.RecordSupport::derivedConstructor(modified
> component names...):(T extends Record, Class<?>... /* modified component
> types */)T]
>
>
> with the `derivedConstructor` bootstrap method:
>
> public static MethodHandle derivedConstructor(MethodHandles.Lookup lookup,
> String unused, MethodType type, String... modifiedComponents) throws
> ReflectiveOperationException {
> requireNonNull(lookup);
> requireNonNull(type);
> // implicit null-check:
> List<String> modifiedComponentNames = List.of(modifiedComponents);
>
> Class<?> rtype = type.returnType();
> if (
> !rtype.isRecord()
> || type.parameterCount() != modifiedComponents.length + 1
> || type.parameterType(0) != rtype
> ) {
> throw new IllegalArgumentException("...");
> }
>
> Set<String> remainingComponentNames = new
> HashSet(modifiedComponentNames);
> if (remainingComponentNames.size() != modifiedComponentNames.size()) {
> throw new IllegalArgumentException("Duplicate component names
> in modifiedComponents");
> }
>
> RecordComponent[] recordComponents = rtype.getRecordComponents();
>
> var componentTypes = new Class<?>[recordComponents.length];
> var filters = new MethodHandle[recordComponents.length];
> var reorder = new int[recordComponents.length];
>
> for (int i = 0, j = 1; i < recordComponents.length; i++) {
> var component = recordComponents[i];
> componentTypes[i] = component.getType();
>
> var getter = lookup.findVirtual(rtype, component.getName(),
> MethodType.methodType(component.getType()));
> if (modifiedComponentNames.contains(component.getName())) {
> remainingComponentNames.remove(component.getName());
> filters[i] = null;
> reorder[i] = j++;
> } else {
> filters[i] = getter;
> reorder[i] = 0;
> }
> }
>
> if (!remainingComponentNames.isEmpty()) {
> throw new IllegalArgumentException("Components " +
> remainingComponentNames + " are not present in the record " + rtype);
> }
>
> var canonicalConstructor = lookup.findConstructor(rtype,
> MethodType.methodType(rtype, componentTypes);
> ...
While this is very smart, I would prefer not to do that. JLS 13.4.27 talks
about binary compatibility for record, and warns that adding or removing
components may break record usages. So I don't think we need to provide too
much support for adding or removing record components.
Overall, I believe the current code adheres to what the JLS draft says. If the
author of the record wants to add or remove components, they can make it so
that the existing compiled code still works (by adding constructor overloads
and keeping the accessors for removed component). It is better, IMO, to keep
the behavior predictable, and leave adjustments after record component
modifications up to the record author, than to introduce some magic.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1549052694
