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.
Approach: Throw more informative error message than NPE with more info.
Screened by: Alex Miller
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)
I believe that's correct - I have updated the contributing page.
The patch I attached yesterday did start from master, so I think it's good to go for review.
Thanks Alex & Andy!
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.
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.
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.