commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: [Math - 403] Never propagate a "NullPointerException" resulting from bad usage of the API
Date Fri, 04 Mar 2011 19:34:58 GMT
On 3/4/11 10:47 AM, Luc Maisonobe wrote:
> Le 04/03/2011 13:55, Gilles Sadowski a écrit :
>>>>>>> SingularMatrixException, NonSymmetricMatrixException and the
likes are
>>>>>>> really tightly bound to linear algebra and could be in the linear
>>>>>>> package where they are triggered. They could appear in the signatures
of
>>>>>>> algorithms in other package that do call linear algebra, but
this is not
>>>>>>> sufficient to put them in a general package.
>>>>>> Yes, the "...MatrixException" classes are the borderline case (meaning
that,
>>>>>> if they were the majority of exception classes, I'd prefer to see
them in
>>>>>> the "linear" package). However, I argue that when there exists an
>>>>>> "exception" to contain the general exceptions, then it is clearer
to have
>>>>>> them all there.
>>>>>> Moreover "NonPositiveDefiniteMatrixException" is already a counter-example
>>>>>> as it is used in "linear" and in "random".
>>>>> It is used in random because the classes in the random package do use
>>>>> the linear package to perform a Cholesky decomposition, which triggers
>>>>> the exception. This is an example of the last sentence in my rationale
>>>>> above. This appearance is not sufficient to put the exception out of
linear.
>>>> The class "CorrelatedRandomVectorGenerator" does not use the
>>>> "CholeskyDecomposition" from package "linear"; it has its own "decompose"
>>>> method which explicitly throws "NonPositiveDefiniteMatrixException"
>>>> instances (at lines 214 and 222).
>>> Right. I did not remember that (despite I wrote this code).
>> So, what is the lesson from having this case? Is the whole method a duplicate
>> that should be replace by a call to something in "linear"? Or is it a proof
>> that some exceptions might be appropriately used outside of "linear"?
I would say that regardless of the implementation choice, the
exception makes sense in this context (variance-covariance matrix
must be positive definite), so does mean it is useful outside of the
linear package, but it is a linear abstraction, so IMO should be
defined there. 

Phil
> I think the method should be moved to a new
> TruncatedCholeskyDecomposition or RectangularCholeskyDecomposition. I
> don't think we can add it has a new implementation or it should be added
> to the existing CholeskyDecomposition because the decomposition matrix
> shape is not triangular.
>
>>>> [...]
>>>>
>>>> I propose to extend the "MathThrowable" interface with methods declarations
>>>> similar to what is done in [Lang] and implement the functionality in
>>>> "MathRuntimeException". [While looking at it, I notice that [Lang] has
>>>> a dedicated "exception" package...]
>>> Fine.
>> I'll create a JIRA issue.
>>
>>>>>>>>>  6) don't provide any top-level exception for errors
occurring in
>>>>>>>>>     user-provided code (for solvers, ode, matrix visitors
...) but
>>>>>>>>>     ask them in documentation to use their own unchecked
exception
>>>>>>>>>     that [math] will never see (i.e. remove FunctionEvaluationException,
>>>>>>>>>     DerivativeException, MatrixVisitorException)
>>>>>>>> +1 for removing all exceptions for which there doesn't exist
any "throw"
>>>>>>>>    statement within CM. And also "MathUserException" (the
few uses of it in
>>>>>>>>    trunk should be replaced).
>>>>>>>>
>>>>>>>>> I'm not sure for NullPointer/NullArgument. Perhaps our
own
>>>>>>>>> NullArgumentException with the [lang] exception context
principle would
>>>>>>>>> be fine. I doubt we should check everything by ourselves
(it would be a
>>>>>>>>> real performance killer), so whatever we choose there
will be untracked
>>>>>>>>> NPE. We should check some arguments where it makes sense,
which is what
>>>>>>>>> we already do at some places.
>>>>>>>> +1 for dropping "NullArgumentException" and letting the JVM
raise NPE.
>>>>>> I'll also create a JIRA issue for reaching a conclusion on this one.
>>>>> OK, I think we agree here. Phil preferred the way we created runtime
>>>>> exceptions as of 2.1, though. Perhaps we could revive a simple
>>>>> createNullPointerException without arguments ?
>>>> But why would one prefer to write
>>>> ---
>>>>   throw MathRuntimeException.createNullPointerException();
>>>> ---
>>>> instead of
>>>> ---
>>>>   throw new NullPointerException();
>>>> ---
>>>> ?
>>> For example to have a single point where to put breakpoints for
>>> debugging. I used that a lot as such exceptions mainly occur during
>>> development phase.
>> Thus, IIUC, this is a developer feature.
>> I understand the convenience, but I wouldn't trade code consistency for it.
>> I'd rather keep "NullArgumentException" (or a "MathNullPointerException" if
>> we want it to extend the standard NPE) so that you can put your breakpoint
>> in the constructor of that exception. Thus you have the convenience without
>> the code asymmetry.
> OK for MathNullPointerException.
>
>>>> Not only is this inconvenient, it is also dangerous: I was bitten more than
>>>> once writing this:
>>>> ---
>>>>   MathRuntimeException.createNullPointerException();
>>>> ---
>>>> which the compiler happily accepted.
>>> Of course, as it would have accepted:
>>>
>>>   new NullPointerException();
>>>
>>> Unfortunately, we cannot put the throw inside the method. Trying to do
>>> this prevent the optimizer to wor properly as it does not know in the
>>> caller that the method would never return, and it brings lots of
>>> warnings from code checking tools for the same reason.
>> The method alternative looses much of its appeal because it doesn't perform
>> the complete action ("throw" included), as I had initially assumed when
>> wrongly leaving out the "throw" statement. I wouldn't have written
>>   new NullPointerException();
>> as, to me, it would have _obviously_ looked wrong.
>>
>> I don't understand the problem with the "optimizer". There are several
>> methods in CM that seem to perform similarly (i.e. they check something and
>> throw an exception). An example is "checkOrder" in "MathUtils".
>> In fact we could add a "checkNotNull" in "MathUtils":
>> ---
>> public checkNotNull(Object obj) {
>>   if (obj == null) {
>>     throw new NullPointerException();
>>   }
>> }
>> ---
>> Wouldn't that satisfy all needs (ability to set a breakpoint and no factory
>> methods)?
> No the check may succeed or not, so sometimes these methods do return.
>
> What I had in mind was something like:
>
>   public static void throwNPE() {
>      throw new MySpecialNPE extends NullPointerException() {
>         // my special code here
>      };
>   }
>
> Such a method *never* returns but on the caller side, the compiler, and
> the checking tools don't know it. Typically when you have code like:
>
>   int var;
>   if (obj != null) {
>     var = obj.getVar();
>   } else {
>     throwNPE();
>   }
>
>   // use var
>
> Then tools will complain that var may not be initialized, which is
> false. However it is only because throwNPE never returns that it is false.
>
> Another way to put it that would probably work is:
>
>   public static void checkNonNull(Object o) {
>     if (o == null) {
>        throw new MySpecialNPE extends NullPointerException() {
>           // my special code here
>        };
>     }
>   }
>
> Then the caller would do:
>
>  checkNonNull(obj);
>  int var = obj.getVar();
>
>
> Luc
>
>>
>> Regards,
>> Gilles
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message