-
Notifications
You must be signed in to change notification settings - Fork 77
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
Blackbird fails to deserialize varargs array #141
Comments
Ok sounds like a bug. I assume it'd likely fail with Afterburner as well. |
Does not look like this fails with Afterburner, for whatever reason. |
@stevenschlansker You might have a better idea of where type check or casting goes wrong here: I added test |
Strange, usually varargs and arrays behave the same, but there are a ton of edge cases. |
Yeah I thought they should be (i.e. just syntactic sugar). But I guess there are specific edge cases. |
I've just encountered this as well. Would you be willing to backport a fix to 2.12.x if I can put something together? I imagine the blackbird module will become increasingly popular as jdk17 LTS is released in a couple months and folks replace afterburner. |
I've created a simple PR which avoids attempting to optimize varargs methods: #145 I'm not sure what dragons lurk in the deep if we attempt to optimize them, however I suspect it's not worthwhile to optimize varargs methods because they're generally expensive due to implicit array allocation, and tend not to be used in performance critical code. |
Thank you for the PR! I still want to take at least one attempt to optimize it, in case it turns out to be easy - but I will definitely borrow the test cases and this is a good fallback option if it turns out to be hard to optimize. |
On backporting: yes, I think this would make sense to backport in some form. Unfortunate timing-wise is that I just pushed 2.12.4 so the fix would likely only get released as a micro-patch (which I can do if there is need). |
Please consider and test #146 which I believe solves this issue. |
The merged version should patch cleanly against 2.12 if that's a priority. |
Backported just the fix -- not sure if or when there might be release but seems reasonable to make that possible at least. |
The following code snippet fails. However, the same snippet works if either Blackbird is removed, or the constructor argument is changed to use
double[]
notation instead ofdouble...
The text was updated successfully, but these errors were encountered: