Skip to content
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

Update list filter to handle arrays properly (#1013) #1014

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

egyedt
Copy link
Contributor

@egyedt egyedt commented Mar 2, 2023

@egyedt
Copy link
Contributor Author

egyedt commented Mar 6, 2023

@mattcoley, @tkindy
Could you please review it and merge it if this is OK for you?

@@ -45,6 +47,34 @@ public Object filter(Object var, JinjavaInterpreter interpreter, String... args)
result = Chars.asList(((String) var).toCharArray());
} else if (Collection.class.isAssignableFrom(var.getClass())) {
result = Lists.newArrayList((Collection<?>) var);
} else if (var.getClass().isArray()) {
if (var instanceof boolean[]) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at this last week and couldn't think of a way in Java to handle these primitive arrays generically other than what you did here to list out all the primitive cases. @tkindy curious if you know of a better way to handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, there is no better way to do that. Most of the other opportunities ends in a list which contains only 1 element, an array. Which is not what we want here. We want to have a list of "n" elements, where the elements coming from an array which length is "n".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe egyedt's correct. As far as I know, there isn't a generic way to handle primitive arrays; like individual primitives, the element type is significant because primitives have different sizes in memory and the type determines how interactions with them are compiled. This contrasts with generic objects like List<T> which only hold references, all of which are the same size in memory.

It seems egyedt is also correct that we need to manually box the primitive arrays before passing to Arrays.asList. Calling Arrays.asList((boolean[]) var) results in a List<boolean[]> rather than a List<Boolean> as we want. This is presumably because Arrays.asList's parameter is typed as T..., where T is inherently a reference type. This clashes with the primitive type of the elements in the array, so the call is resolved as being passed one boolean[] (a reference type) parameter instead of N boolean (a primitive type) parameters.

So I think this is the best we can do. 👍

Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my end but just want to wait on @tkindy's thoughts first before merging

Copy link
Contributor

@tkindy tkindy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clean and well-tested PR, @egyedt!

@@ -45,6 +47,34 @@ public Object filter(Object var, JinjavaInterpreter interpreter, String... args)
result = Chars.asList(((String) var).toCharArray());
} else if (Collection.class.isAssignableFrom(var.getClass())) {
result = Lists.newArrayList((Collection<?>) var);
} else if (var.getClass().isArray()) {
if (var instanceof boolean[]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe egyedt's correct. As far as I know, there isn't a generic way to handle primitive arrays; like individual primitives, the element type is significant because primitives have different sizes in memory and the type determines how interactions with them are compiled. This contrasts with generic objects like List<T> which only hold references, all of which are the same size in memory.

It seems egyedt is also correct that we need to manually box the primitive arrays before passing to Arrays.asList. Calling Arrays.asList((boolean[]) var) results in a List<boolean[]> rather than a List<Boolean> as we want. This is presumably because Arrays.asList's parameter is typed as T..., where T is inherently a reference type. This clashes with the primitive type of the elements in the array, so the call is resolved as being passed one boolean[] (a reference type) parameter instead of N boolean (a primitive type) parameters.

So I think this is the best we can do. 👍

@tkindy tkindy merged commit 1239b20 into HubSpot:master Mar 6, 2023
@akatona84
Copy link
Contributor

akatona84 commented Mar 7, 2023

Hey folks, any chance you'll be releasing a new version in the near future? I see it's in progress, thx! #1025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants