RFR 8146458 Improve exception reporting for Objects.checkIndex/checkFromToIndex/checkFromIndexSize

[Stuart Marks]

Hi Paul,

I agree with John on a couple points, namely the removal of the two-arg 
constructors from the exception types, and the use of a term like "factory" 
instead of "mapper" for the exception-instance-producing functions. An 
alternative might be "supplier" (though that might cause confusion with the 
Supplier functional interface).

I'm concerned about a few other things though. I spent some time going over the 
change this morning and it took a while to digest. My hunch is that for what 
this is trying to do, it could be simpler, though I haven't yet fully absorbed 
all of what's going on here to come up with concrete suggestions for how to do so.

One area in particular is the message formatter functions. There are really 
three cases smashed into one:

     KIND                    ARGS

     checkIndex              index, length
     checkFromToIndex        from, to, length
     checkFromIndexSize      from, size, length

I think, in order to avoid introducing new functional interfaces, this 
information is funneled into a single varargs call, which has to use Integer 
boxes, because it then wraps its args into a List (thanks for using the new List 
factory!), which then has to unpack the list and check its length against the 
number of arguments for the particular message kind.

I understand that this is the "slow" exception-throwing path, but my sense is 
still that this ought to be made simpler. Perhaps the scope of what it's trying 
to accomplished should be reduced, if that helps things.

I'll continue to think about this further.

I also have a naming bikeshed but that can be postponed. :-)

s'marks


On 1/15/16 1:24 PM, John Rose wrote:
> On Jan 11, 2016, at 7:04 AM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>>
>> When the new range check methods Object.check* were added the exception reporting was a little vague and lossy, but i got a CCC pass to revisit later on, and John nudged me to sort this out, so here is another more reflective approach:
>>
>>   http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8146458-checkIndex-exception-reporting/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8146458-checkIndex-exception-reporting/webrev/>
>>   http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8146458-checkIndex-exception-reporting/specdiff/overview-summary.html <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8146458-checkIndex-exception-reporting/specdiff/overview-summary.html>
>
> I like it.  (No surprise there.)
>
> In the new API point the term "mapper", and "exception mapping function" in the existing
> API points, doesn't really convey much  to me.
>
> The main cog. diss. is that, at the time the EMF is called, there is no exception created yet,
> although it is true that an exceptional condition has been identifier.  But the exception thrown
> is the first to be created; it is not "mapped" from a previous exception.
>
> So I'd prefer "factory", personally:  "factory", "exception factory [function]".
> A T-factory maps () to T, while a T-mapper maps (T) to T.
>
>> ...
>>
>> The two int arg constructors added to Array/String/IndexOutOfBoundsException have been removed.
>
> Yes; that pays for the new API point, IMO.  Very nice.
>
> One nit:
>
> -+        // Switch to default if fewer arguments than required are supplied
> -+        switch ((args.size() < argSize) ? "" : checkKind) {
> ++        // Switch to default if wrong number of arguments is supplied
> ++        switch ((args.size() != argSize) ? "" : checkKind) {
>
> That is, excess values should show up, so they can be debugged.
> (Unless you have a use case in mind?)
>
> Would you mind including a test case which exercises, say,
> ArrayIndexOutOfBoundsException::new?  Might as well
> make sure those wires connect properly; test the class of
> the thrown exception as well as the message.
>
>> I did ponder about adding a new constructors with String, List<Integer> arguments. I could still do that, but that does spread out the notion beyond that of Objects and i prefer that it be contained.
>
> Nope.  Keep this thing localized.
>
> Reviewed, if you need a reviewer.
>
> — John
>