vector-of throws NullPointerException if given unrecognized type

Description

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

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

3

Activity

Show:

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.

Completed

Details

Assignee

Reporter

Approval

Ok

Patch

Code and Test

Priority

Affects versions

Fix versions

Created April 15, 2015 at 1:40 AM
Updated September 7, 2017 at 10:25 PM
Resolved September 7, 2017 at 10:25 PM