-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Non-boxing 'addAll' Methods for Primitive Types #2389
Conversation
conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java
Show resolved
Hide resolved
@@ -56,6 +56,14 @@ public boolean addAll(int index, Collection<? extends SafeLong> collection) { | |||
return delegate.addAllAtIndex(index, target); | |||
} | |||
|
|||
public void addAll(long... source) { | |||
for (long value : source) { | |||
// Doesn't use SafeLong creation because this causes unnecessary boxing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is a little too specific and doesn't explain how the underlying type holds longs and those must adhere to the abstraction.
*/ | ||
// This method modifies a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set | ||
public static void addAllToDoubleList(Collection<Double> addTo, double[] elementsToAdd) { | ||
if (addTo instanceof ConjureDoubleList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An inelegant solution to preserve the feedback on #2307 (comment), but still capture the optimization we added. Also has some prior art in addAll
.
conjure-lib/src/main/java/com/palantir/conjure/java/lib/SafeLong.java
Outdated
Show resolved
Hide resolved
LIST("List", false), | ||
DOUBLE_LIST("DoubleList", true), | ||
INTEGER_LIST("IntegerList", true), | ||
BOOLEAN_LIST("BooleanList", true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also maybe not? #2390
@@ -66,7 +100,7 @@ public static MethodSpec.Builder createCollectionSetterBuilder( | |||
field.name)); | |||
} | |||
|
|||
public static MethodSpec.Builder createOptionalSetterBuilder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can extract this into a separate PR, but I noticed that we were overly exposing these methods which in turn was overly exposing the EnrichedField
type.
df23c89
to
5590778
Compare
@@ -767,10 +787,19 @@ private List<MethodSpec> createAuxiliarySetters(EnrichedField enriched, boolean | |||
Type type = enriched.conjureDef().getType(); | |||
Optional<LogSafety> safety = safetyEvaluator.getUsageTimeSafety(enriched.conjureDef()); | |||
|
|||
ImmutableList.Builder<MethodSpec> builder = ImmutableList.builder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this a builder so I could optionally add another method and not have to duplicate logic to return lists directly.
5590778
to
b021276
Compare
conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java
Outdated
Show resolved
Hide resolved
conjure-java-core/src/integrationInput/java/com/palantir/product/SafeLongExample.java
Outdated
Show resolved
Hide resolved
b021276
to
284c2ca
Compare
conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java
Outdated
Show resolved
Hide resolved
conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureIntegerList.java
Outdated
Show resolved
Hide resolved
284c2ca
to
86e4def
Compare
86e4def
to
a1f407a
Compare
Before this PR
This is an alternative variant of #2312 which attempts to fix the boxing overhead merely introduced by the shape of our API (primitive types on both sides of the abstraction). However, there was some objection to that approach due to some stylistic and API design disagreements. This alternative seeks to remedy some of those concerns by following some "prior art" from libraries we accept / use all the time - Immutables.
Immutables will generate a vararg
addAll
variant for aList
collection of some primitive boxed type. For example,List<Double>
will produce something like:After this PR
This change first punches a hole through from the underlying eclipse-collections types e.g.
DoubleList
which has routines likepublic boolean addAll(double... source)
which allow for direct, non-boxed, additions to the type.Then, taking Immutables as inspiration, I wired up our codegen to leverage this abstraction. This alternative design addresses the mutability concerns as this API clearly communicates how data will be owned moving forward (i.e. copied). This implementation also addresses the concerns of having a setter that resets the type may cause order of operations bugs in the field while populating the builder.
Of Note
Yes... I am aware that the
Iterable
variant is a setter in the builder e.g.:This feels like a design oversight (IMO) seeing as how
primitiveItems
is already initialized statically andnewNonNullIntegerList
simply performs a copy usingaddAll
. Removing it from the field will be considerable work, so this will need to be inconsistent for the time being. And also previous design deficiencies cannot justify future designs.Tangent
Sorry if this is getting off topic, but that
Iterable
variant seems destined to be used incorrectly. Imagine the following code:Either way, necessary or not, I do not love the invalid state that is further representable on this API by adding an additional similarly named setter.