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 <[email protected]> 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
