From a8a1f1d39bb2ab88d8af40096c98e91dd91ffc1b Mon Sep 17 00:00:00 2001 From: Shibi Balamurugan Date: Tue, 21 May 2024 15:04:38 -0400 Subject: [PATCH 1/5] Add primitive array setter optimization --- .../com/palantir/product/ListExample.java | 14 ++++ .../com/palantir/product/SafeLongExample.java | 7 ++ .../java/types/BeanBuilderGenerator.java | 71 ++++++++++++++++--- .../java/lib/internal/ConjureCollections.java | 44 ++++++++++-- 4 files changed, 119 insertions(+), 17 deletions(-) diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java index 2a3c0d33f..eacbdda3f 100644 --- a/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java +++ b/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java @@ -218,6 +218,13 @@ public Builder primitiveItems(@Nonnull Iterable primitiveItems) { return this; } + public Builder primitiveItems(@Nonnull int[] primitiveItems) { + checkNotBuilt(); + this.primitiveItems = ConjureCollections.newNonNullIntegerList( + Preconditions.checkNotNull(primitiveItems, "primitiveItems cannot be null")); + return this; + } + public Builder addAllPrimitiveItems(@Nonnull Iterable primitiveItems) { checkNotBuilt(); ConjureCollections.addAllAndCheckNonNull( @@ -240,6 +247,13 @@ public Builder doubleItems(@Nonnull Iterable doubleItems) { return this; } + public Builder doubleItems(@Nonnull double[] doubleItems) { + checkNotBuilt(); + this.doubleItems = ConjureCollections.newNonNullDoubleList( + Preconditions.checkNotNull(doubleItems, "doubleItems cannot be null")); + return this; + } + public Builder addAllDoubleItems(@Nonnull Iterable doubleItems) { checkNotBuilt(); ConjureCollections.addAllAndCheckNonNull( diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/SafeLongExample.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/SafeLongExample.java index ba755c61a..fd3df5f25 100644 --- a/conjure-java-core/src/integrationInput/java/com/palantir/product/SafeLongExample.java +++ b/conjure-java-core/src/integrationInput/java/com/palantir/product/SafeLongExample.java @@ -135,6 +135,13 @@ public Builder safeLongList(@Nonnull Iterable safeLongList) { return this; } + public Builder safeLongList(@Nonnull long[] safeLongList) { + checkNotBuilt(); + this.safeLongList = ConjureCollections.newNonNullSafeLongList( + Preconditions.checkNotNull(safeLongList, "safeLongList cannot be null")); + return this; + } + public Builder addAllSafeLongList(@Nonnull Iterable safeLongList) { checkNotBuilt(); ConjureCollections.addAllAndCheckNonNull( diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java index 0f2566558..1e918cda3 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java @@ -28,6 +28,7 @@ import com.google.errorprone.annotations.CheckReturnValue; import com.palantir.conjure.java.ConjureAnnotations; import com.palantir.conjure.java.Options; +import com.palantir.conjure.java.lib.SafeLong; import com.palantir.conjure.java.lib.internal.ConjureCollections; import com.palantir.conjure.java.types.BeanGenerator.EnrichedField; import com.palantir.conjure.java.util.JavaNameSanitizer; @@ -57,6 +58,7 @@ import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeIllegalStateException; import com.squareup.javapoet.AnnotationSpec; +import com.squareup.javapoet.ArrayTypeName; import com.squareup.javapoet.ClassName; import com.squareup.javapoet.CodeBlock; import com.squareup.javapoet.FieldSpec; @@ -671,6 +673,36 @@ private MethodSpec createCollectionSetter(String prefix, EnrichedField enriched, .build(); } + private MethodSpec createPrimitiveCollectionSetter(EnrichedField enriched) { + FieldSpec field = enriched.poetSpec(); + Type type = enriched.conjureDef().getType(); + + Type innerType = type.accept(TypeVisitor.LIST).getItemType(); + TypeName boxedTypeName = typeMapper.getClassName(innerType); + TypeName innerTypeName; + // SafeLong is just a special case of long + if (boxedTypeName.equals(ClassName.get(SafeLong.class))) { + innerTypeName = ConjureAnnotations.withSafety( + TypeName.LONG, safetyEvaluator.getUsageTimeSafety(enriched.conjureDef())); + } else { + innerTypeName = ConjureAnnotations.withSafety( + Primitives.unbox(boxedTypeName), safetyEvaluator.getUsageTimeSafety(enriched.conjureDef())); + } + CollectionType collectionType = getCollectionType(type); + MethodSpec.Builder setterBuilder = BeanBuilderAuxiliarySettersUtils.publicSetter(enriched, builderClass) + .addParameter(Parameters.nonnullParameter(ArrayTypeName.of(innerTypeName), field.name)) + .addCode(verifyNotBuilt()) + .addCode(CodeBlocks.statement( + "this.$1N = $2T.newNonNull$3L($4L)", + enriched.poetSpec().name, + ConjureCollections.class, + collectionType.getConjureCollectionType().getCollectionName(), + Expressions.requireNonNull( + enriched.poetSpec().name, enriched.fieldName().get() + " cannot be null"))); + + return setterBuilder.addStatement("return this").build(); + } + @SuppressWarnings("checkstyle:CyclomaticComplexity") private CodeBlock typeAwareAssignment(EnrichedField enriched, Type type, boolean shouldClearFirst) { FieldSpec spec = enriched.poetSpec(); @@ -766,9 +798,18 @@ private List createAuxiliarySetters(EnrichedField enriched, boolean Optional safety = safetyEvaluator.getUsageTimeSafety(enriched.conjureDef()); if (type.accept(TypeVisitor.IS_LIST)) { - return ImmutableList.of( - createCollectionSetter("addAll", enriched, override), - createItemSetter(enriched, type.accept(TypeVisitor.LIST).getItemType(), override, safety)); + CollectionType collectionType = getCollectionType(type); + if (collectionType.getConjureCollectionType().isPrimitiveCollection() + && collectionType.useNonNullFactory()) { + return ImmutableList.of( + createPrimitiveCollectionSetter(enriched), + createCollectionSetter("addAll", enriched, override), + createItemSetter(enriched, type.accept(TypeVisitor.LIST).getItemType(), override, safety)); + } else { + return ImmutableList.of( + createCollectionSetter("addAll", enriched, override), + createItemSetter(enriched, type.accept(TypeVisitor.LIST).getItemType(), override, safety)); + } } if (type.accept(TypeVisitor.IS_SET)) { @@ -820,7 +861,9 @@ private CodeBlock optionalAssignmentStatement(EnrichedField enriched, OptionalTy private static final EnumSet OPTIONAL_PRIMITIVES = EnumSet.of(PrimitiveType.Value.INTEGER, PrimitiveType.Value.DOUBLE, PrimitiveType.Value.BOOLEAN); - /** Check if the optionalType contains a primitive boolean, double or integer. */ + /** + * Check if the optionalType contains a primitive boolean, double or integer. + */ private boolean isPrimitiveOptional(OptionalType optionalType) { return optionalType.getItemType().accept(TypeVisitor.IS_PRIMITIVE) && OPTIONAL_PRIMITIVES.contains( @@ -1044,22 +1087,28 @@ public boolean useNonNullFactory() { } private enum ConjureCollectionType { - LIST("List"), - DOUBLE_LIST("DoubleList"), - INTEGER_LIST("IntegerList"), - BOOLEAN_LIST("BooleanList"), - SAFE_LONG_LIST("SafeLongList"), - SET("Set"); + LIST("List", false), + DOUBLE_LIST("DoubleList", true), + INTEGER_LIST("IntegerList", true), + BOOLEAN_LIST("BooleanList", true), + SAFE_LONG_LIST("SafeLongList", true), + SET("Set", false); private final String collectionName; + private final Boolean primitiveCollection; - ConjureCollectionType(String collectionName) { + ConjureCollectionType(String collectionName, Boolean primitiveCollection) { this.collectionName = collectionName; + this.primitiveCollection = primitiveCollection; } public String getCollectionName() { return collectionName; } + + public Boolean isPrimitiveCollection() { + return primitiveCollection; + } } private enum ConjureCollectionNullHandlingMode { diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java index 92b3b7b81..770ac8c25 100644 --- a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java +++ b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java @@ -18,6 +18,7 @@ import com.palantir.conjure.java.lib.SafeLong; import com.palantir.logsafe.Preconditions; +import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashSet; @@ -140,11 +141,22 @@ public static Set newNonNullSet(Iterable iterable) { return set; } + /** + * The following Conjure boxed list wrappers for the eclipse-collections [type]ArrayList are temporary (except + * ConjureSafeLongList). In eclipse-collections 12, a BoxedMutable[type]List will be released. Once available, + * Conjure[type]List should be replaced with that. + */ + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullDoubleList() { return new ConjureDoubleList(new DoubleArrayList()); } + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set + public static List newNonNullDoubleList(double[] doubles) { + return new ConjureDoubleList(DoubleArrayList.newListWith(doubles)); + } + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullDoubleList(Iterable iterable) { List doubleList; @@ -158,17 +170,16 @@ public static List newNonNullDoubleList(Iterable iterable) { return doubleList; } - /** - * The following Conjure boxed list wrappers for the eclipse-collections [type]ArrayList are temporary (except - * ConjureSafeLongList). In eclipse-collections 12, a BoxedMutable[type]List will be released. Once available, - * Conjure[type]List should be replaced with that. - */ - // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullIntegerList() { return new ConjureIntegerList(new IntArrayList()); } + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set + public static List newNonNullIntegerList(int[] integers) { + return new ConjureIntegerList(IntArrayList.newListWith(integers)); + } + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullIntegerList(Iterable iterable) { List integerList; @@ -187,6 +198,11 @@ public static List newNonNullBooleanList() { return new ConjureBooleanList(new BooleanArrayList()); } + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set + public static List newNonNullBooleanList(boolean[] booleans) { + return new ConjureBooleanList(BooleanArrayList.newListWith(booleans)); + } + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullBooleanList(Iterable iterable) { List booleanList; @@ -205,6 +221,18 @@ public static List newNonNullSafeLongList() { return new ConjureSafeLongList(new LongArrayList()); } + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set + public static List newNonNullSafeLongList(long[] longs) { + for (long value : longs) { + if (!safeLongCheck(value)) { + throw new SafeIllegalArgumentException( + "number must be safely representable in javascript i.e. lie between -9007199254740991 and " + + "9007199254740991"); + } + } + return new ConjureSafeLongList(LongArrayList.newListWith(longs)); + } + // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullSafeLongList(Iterable iterable) { List safeLongList; @@ -217,4 +245,8 @@ public static List newNonNullSafeLongList(Iterable iterable) return safeLongList; } + + private static boolean safeLongCheck(long value) { + return SafeLong.MIN_VALUE.longValue() <= value && value <= SafeLong.MAX_VALUE.longValue(); + } } From 7ba826e3649a2bb80678dade1634b1f26984c981 Mon Sep 17 00:00:00 2001 From: Shibi Balamurugan Date: Tue, 21 May 2024 15:11:11 -0400 Subject: [PATCH 2/5] Add tests --- .../lib/internal/ConjureCollectionsTest.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java b/conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java index 1e03e890e..222f86dff 100644 --- a/conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java +++ b/conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java @@ -17,8 +17,10 @@ package com.palantir.conjure.java.lib.internal; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import com.palantir.conjure.java.lib.SafeLong; +import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import java.util.List; import org.junit.jupiter.api.Test; @@ -51,6 +53,12 @@ public void newNonNullDoubleList() { doubleList.clear(); assertThat(doubleList).hasSize(0); + + doubleList = ConjureCollections.newNonNullDoubleList(new double[] {0.1, 0.2, 0.3}); + assertThat(doubleList).hasSize(3); + assertThat(doubleList.get(0)).isEqualTo(0.1); + assertThat(doubleList.get(1)).isEqualTo(0.2); + assertThat(doubleList.get(2)).isEqualTo(0.3); } @Test @@ -81,5 +89,15 @@ public void newNonNullSafeLongList() { safeLongList.clear(); assertThat(safeLongList).hasSize(0); + + safeLongList = ConjureCollections.newNonNullSafeLongList(new long[] {1L, 2L, 3L}); + assertThat(safeLongList).hasSize(3); + assertThat(safeLongList.get(0)).isEqualTo(SafeLong.of(1L)); + assertThat(safeLongList.get(1)).isEqualTo(SafeLong.of(2L)); + assertThat(safeLongList.get(2)).isEqualTo(SafeLong.of(3L)); + + assertThatExceptionOfType(SafeIllegalArgumentException.class) + .isThrownBy(() -> ConjureCollections.newNonNullSafeLongList( + new long[] {1L, 2L, SafeLong.MAX_VALUE.longValue() + 1})); } } From dee84cb21c4fda50850bf8eb0ad8b20c69fc0a18 Mon Sep 17 00:00:00 2001 From: Shibi Balamurugan Date: Fri, 14 Jun 2024 15:29:04 -0400 Subject: [PATCH 3/5] Add array setter with feature flag --- .../src/main/java/com/palantir/conjure/java/Options.java | 9 +++++++++ .../conjure/java/types/BeanBuilderGenerator.java | 3 ++- .../conjure/java/types/ObjectGeneratorTests.java | 1 + .../com/palantir/conjure/java/cli/ConjureJavaCli.java | 7 +++++++ 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/Options.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/Options.java index fd526ff8e..472c40aa4 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/Options.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/Options.java @@ -178,6 +178,15 @@ default boolean primitiveOptimizedCollections() { return false; } + /** + * When set, enables codegen for array setters for primitive optimized collections. + * This feature is experimental and subject to change. + */ + @Value.Default + default boolean primitiveCollectionArraySetters() { + return false; + } + Optional packagePrefix(); Optional apiVersion(); diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java index 1e918cda3..754974854 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java @@ -800,7 +800,8 @@ private List createAuxiliarySetters(EnrichedField enriched, boolean if (type.accept(TypeVisitor.IS_LIST)) { CollectionType collectionType = getCollectionType(type); if (collectionType.getConjureCollectionType().isPrimitiveCollection() - && collectionType.useNonNullFactory()) { + && collectionType.useNonNullFactory() + && options.primitiveCollectionArraySetters()) { return ImmutableList.of( createPrimitiveCollectionSetter(enriched), createCollectionSetter("addAll", enriched, override), diff --git a/conjure-java-core/src/test/java/com/palantir/conjure/java/types/ObjectGeneratorTests.java b/conjure-java-core/src/test/java/com/palantir/conjure/java/types/ObjectGeneratorTests.java index a855607c6..1bc1a3a3e 100644 --- a/conjure-java-core/src/test/java/com/palantir/conjure/java/types/ObjectGeneratorTests.java +++ b/conjure-java-core/src/test/java/com/palantir/conjure/java/types/ObjectGeneratorTests.java @@ -67,6 +67,7 @@ public void testObjectGenerator_allExamples() throws IOException { .unionsWithUnknownValues(true) .jetbrainsContractAnnotations(true) .primitiveOptimizedCollections(true) + .primitiveCollectionArraySetters(true) .build()))) .emit(def, tempDir); diff --git a/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java b/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java index 7c4a660f7..36ebf428d 100644 --- a/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java +++ b/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java @@ -235,6 +235,12 @@ public static final class GenerateCommand implements Runnable { description = "Enables codegen for primitive optimized collections.") private boolean primitiveOptimizedCollections; + @CommandLine.Option( + names = "--primitiveCollectionArraySetters", + defaultValue = "false", + description = "Ecodegen for array setters for primitive optimized collections.") + private boolean primitiveCollectionArraySetters; + @SuppressWarnings("unused") @CommandLine.Unmatched private List unmatchedOptions; @@ -304,6 +310,7 @@ CliConfiguration getConfiguration() { .unionsWithUnknownValues(unionsWithUnknownValues) .externalFallbackTypes(externalFallbackTypes) .primitiveOptimizedCollections(primitiveOptimizedCollections) + .primitiveCollectionArraySetters(primitiveCollectionArraySetters) .build()) .build(); } From 1bc25e4eb283c6a53980a818c4c784bd1fe4652c Mon Sep 17 00:00:00 2001 From: Sander Kromwijk Date: Mon, 23 Sep 2024 10:50:13 -0500 Subject: [PATCH 4/5] Copy list defensively --- .../java/lib/internal/ConjureCollections.java | 18 +++++++++++++----- .../lib/internal/ConjureCollectionsTest.java | 13 +++++++++++-- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java index 770ac8c25..56400a5b2 100644 --- a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java +++ b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java @@ -154,7 +154,9 @@ public static List newNonNullDoubleList() { // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullDoubleList(double[] doubles) { - return new ConjureDoubleList(DoubleArrayList.newListWith(doubles)); + double[] conjureCopy = new double[doubles.length]; + System.arraycopy(doubles, 0, conjureCopy, 0, doubles.length); + return new ConjureDoubleList(DoubleArrayList.newListWith(conjureCopy)); } // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set @@ -177,7 +179,9 @@ public static List newNonNullIntegerList() { // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullIntegerList(int[] integers) { - return new ConjureIntegerList(IntArrayList.newListWith(integers)); + int[] conjureCopy = new int[integers.length]; + System.arraycopy(integers, 0, conjureCopy, 0, integers.length); + return new ConjureIntegerList(IntArrayList.newListWith(conjureCopy)); } // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set @@ -200,7 +204,9 @@ public static List newNonNullBooleanList() { // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullBooleanList(boolean[] booleans) { - return new ConjureBooleanList(BooleanArrayList.newListWith(booleans)); + boolean[] conjureCopy = new boolean[booleans.length]; + System.arraycopy(booleans, 0, conjureCopy, 0, booleans.length); + return new ConjureBooleanList(BooleanArrayList.newListWith(conjureCopy)); } // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set @@ -223,14 +229,16 @@ public static List newNonNullSafeLongList() { // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullSafeLongList(long[] longs) { - for (long value : longs) { + long[] conjureCopy = new long[longs.length]; + System.arraycopy(longs, 0, conjureCopy, 0, longs.length); + for (long value : conjureCopy) { if (!safeLongCheck(value)) { throw new SafeIllegalArgumentException( "number must be safely representable in javascript i.e. lie between -9007199254740991 and " + "9007199254740991"); } } - return new ConjureSafeLongList(LongArrayList.newListWith(longs)); + return new ConjureSafeLongList(LongArrayList.newListWith(conjureCopy)); } // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set diff --git a/conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java b/conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java index 222f86dff..446ca4f83 100644 --- a/conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java +++ b/conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java @@ -54,11 +54,16 @@ public void newNonNullDoubleList() { doubleList.clear(); assertThat(doubleList).hasSize(0); - doubleList = ConjureCollections.newNonNullDoubleList(new double[] {0.1, 0.2, 0.3}); + double[] rawList = new double[] {0.1, 0.2, 0.3}; + doubleList = ConjureCollections.newNonNullDoubleList(rawList); assertThat(doubleList).hasSize(3); assertThat(doubleList.get(0)).isEqualTo(0.1); assertThat(doubleList.get(1)).isEqualTo(0.2); assertThat(doubleList.get(2)).isEqualTo(0.3); + + // Check we made a copy of the array + rawList[0] = 0.4; + assertThat(doubleList.get(2)).isEqualTo(0.3); } @Test @@ -90,12 +95,16 @@ public void newNonNullSafeLongList() { safeLongList.clear(); assertThat(safeLongList).hasSize(0); - safeLongList = ConjureCollections.newNonNullSafeLongList(new long[] {1L, 2L, 3L}); + long[] rawList = new long[] {1L, 2L, 3L}; + safeLongList = ConjureCollections.newNonNullSafeLongList(rawList); assertThat(safeLongList).hasSize(3); assertThat(safeLongList.get(0)).isEqualTo(SafeLong.of(1L)); assertThat(safeLongList.get(1)).isEqualTo(SafeLong.of(2L)); assertThat(safeLongList.get(2)).isEqualTo(SafeLong.of(3L)); + rawList[0] = 42L; + assertThat(safeLongList.get(0)).isEqualTo(SafeLong.of(1L)); + assertThatExceptionOfType(SafeIllegalArgumentException.class) .isThrownBy(() -> ConjureCollections.newNonNullSafeLongList( new long[] {1L, 2L, SafeLong.MAX_VALUE.longValue() + 1})); From de283009effcd9b1f8baa231dc30980b8e6b63d8 Mon Sep 17 00:00:00 2001 From: Sander Kromwijk Date: Mon, 23 Sep 2024 12:55:48 -0500 Subject: [PATCH 5/5] Use clone instead of arraycopy --- .../java/lib/internal/ConjureCollections.java | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java index 56400a5b2..457df38d3 100644 --- a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java +++ b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java @@ -154,9 +154,7 @@ public static List newNonNullDoubleList() { // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullDoubleList(double[] doubles) { - double[] conjureCopy = new double[doubles.length]; - System.arraycopy(doubles, 0, conjureCopy, 0, doubles.length); - return new ConjureDoubleList(DoubleArrayList.newListWith(conjureCopy)); + return new ConjureDoubleList(DoubleArrayList.newListWith(doubles.clone())); } // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set @@ -179,9 +177,7 @@ public static List newNonNullIntegerList() { // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullIntegerList(int[] integers) { - int[] conjureCopy = new int[integers.length]; - System.arraycopy(integers, 0, conjureCopy, 0, integers.length); - return new ConjureIntegerList(IntArrayList.newListWith(conjureCopy)); + return new ConjureIntegerList(IntArrayList.newListWith(integers.clone())); } // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set @@ -204,9 +200,7 @@ public static List newNonNullBooleanList() { // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullBooleanList(boolean[] booleans) { - boolean[] conjureCopy = new boolean[booleans.length]; - System.arraycopy(booleans, 0, conjureCopy, 0, booleans.length); - return new ConjureBooleanList(BooleanArrayList.newListWith(conjureCopy)); + return new ConjureBooleanList(BooleanArrayList.newListWith(booleans.clone())); } // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set @@ -229,8 +223,7 @@ public static List newNonNullSafeLongList() { // This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static List newNonNullSafeLongList(long[] longs) { - long[] conjureCopy = new long[longs.length]; - System.arraycopy(longs, 0, conjureCopy, 0, longs.length); + long[] conjureCopy = longs.clone(); for (long value : conjureCopy) { if (!safeLongCheck(value)) { throw new SafeIllegalArgumentException(