vector-of throws NullPointerException if given unrecognized type
Description
Environment
MacOS X Version 10.9.5
java version "1.8.0_11"
Java(TM) SE Runtime Environment (build 1.8.0_11-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.11-b03, mixed mode)
Attachments
Activity

Johan Mena May 6, 2015 at 5:04 PM
Haha, you shouldn't have bothered, but thanks!
And thanks for the detailed explanation! Actually 'get' was my first approach too but the (get map key not-found) form kept throwing the exception in the `not-found` part even in the happy case. I asked around and found out the else part needs to be evaluated in this case too. Someone suggested using a macro but I'm not very familiar with that just yet, so I went with if-let, which seemed readable. Just out of curiosity, when you tried the get approach, did you use (get map key) and check for nil to throw the exception?
I ran my code through the irc channel and got positive feedback, so completely forgot about performance. Will keep it in mind next time.

Alex Miller May 6, 2015 at 4:02 PM
Ok, I split up the patch into test and code commits so you have credit for some of the changes! Since it's got my stuff in here, this will have to wait till it gets added to 1.8 to get screened by someone else.

Alex Miller May 6, 2015 at 3:56 PM
Ok, I looked at the patch. Functionally, seems ok other than that the find+destructuing doesn't seem to have a use over get.
However, I also looked at performance for the happy path using criterium and the following test:
Before the patch: 27.418350 ns
After the patch: 79.883695 ns
The reason for this is that the prior code created a single map and constructed the array managers in there once, whereas the patch causes the array manager to be created each time through the code.
Re-extracting the map and replacing the find with get, I was back to: 34.320916 ns
I think that's too much of a hit for this change so I looked at a couple variants:
def'ing each am separately and using a `case` expression - 36.566750 ns
using `or` instead of if-let - 29.680252 ns
I think that last one is pretty close, but we're paying a hit just to invoke the new function, so my final stab was turning that into a macro - 28.411626 ns, which seems tolerable to me. Because I used a macro, I also had to move the type hints in vector-of to avoid reflection.
Since I had everything sitting here, I went ahead and made a new patch. I feel bad about replacing yours, but not sure what else makes sense to do.

Johan Mena May 5, 2015 at 7:57 PM
The patch I attached yesterday did start from master, so I think it's good to go for review.
Thanks Alex & Andy!

Alex Miller May 5, 2015 at 4:35 PM
I believe that's correct - I have updated the contributing page.
Details
Assignee
UnassignedUnassignedReporter
importimportApproval
OkPatch
Code and TestPriority
MinorAffects versions
Fix versions
Details
Details
Assignee
Reporter

Summary:
If the user passes an unrecognized type keyword to vector-of, it will throw a NullPointerException with no message. This gives no indication to the user of what the problem is, which can frustrate the user and make debugging harder than it needs to be.
Example:
Approach: Throw more informative error message than NPE with more info.
After:
Patch: clj-1705-3.patch
Screened by: Alex Miller