commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sébastien Brisard <sebastien.bris...@m4x.org>
Subject Re: [Math] About "NullArgumentException" (Was: svn commit: r1381284 - in /commons/proper/math/trunk/...)
Date Thu, 06 Sep 2012 11:58:18 GMT
Hi Luc,

2012/9/6 luc <luc@spaceroots.org>:
> Hi all,
>
> Le 2012-09-06 07:08, Sébastien Brisard a écrit :
>
>> Hi Gilles,
>>
>> 2012/9/6 sebb <sebbaz@gmail.com>:
>>>
>>> On 5 September 2012 22:46, Gilles Sadowski <gilles@harfang.homelinux.org>
>>> wrote:
>>>>
>>>> On Wed, Sep 05, 2012 at 06:30:08PM -0000, luc@apache.org wrote:
>>>>>
>>>>> Author: luc
>>>>> Date: Wed Sep  5 18:30:08 2012
>>>>> New Revision: 1381284
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1381284&view=rev
>>>>> Log:
>>>>> Added throw declarations for package complex.
>>>>>
>>>>> Modified:
>>>>>
>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/complex/Complex.java
>>>>>
>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/complex/ComplexFormat.java
>>>>>
>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/complex/ComplexUtils.java
>>>>>
>>>>> Modified:
>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/complex/Complex.java
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math3/complex/Complex.java?rev=1381284&r1=1381283&r2=1381284&view=diff
>>>>>
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/complex/Complex.java
>>>>> (original)
>>>>> +++
>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/complex/Complex.java
>>>>> Wed Sep  5 18:30:08 2012
>>>>> @@ -22,6 +22,7 @@ import java.util.ArrayList;
>>>>>  import java.util.List;
>>>>>
>>>>>  import org.apache.commons.math3.FieldElement;
>>>>> +import org.apache.commons.math3.exception.MathInternalError;
>>>>>  import org.apache.commons.math3.exception.NullArgumentException;
>>>>>  import org.apache.commons.math3.exception.NotPositiveException;
>>>>>  import org.apache.commons.math3.exception.util.LocalizedFormats;
>>>>> @@ -566,12 +567,17 @@ public class Complex implements FieldEle
>>>>>       * @since 1.2
>>>>>       */
>>>>>      public Complex acos() {
>>>>> -        if (isNaN) {
>>>>> -            return NaN;
>>>>> -        }
>>>>> +        try {
>>>>> +            if (isNaN) {
>>>>> +                return NaN;
>>>>> +            }
>
>
> You are right. It's the result of a bad copy-paste.
>
>
>>>
>>> Regardless of whether it makes sense to catch NUA the above condition
>>> should not be part of the try clause.
>>>
>>>>>
>>>>> -        return this.add(this.sqrt1z().multiply(I)).log()
>>>>> -            .multiply(I.negate());
>>>>> +            return this.add(this.sqrt1z().multiply(I)).log()
>>>>> +                    .multiply(I.negate());
>>>>> +        } catch (NullArgumentException e) {
>>>>> +            // this should never happen as intermediat results are not
>>>>> null
>>>>> +            throw new MathInternalError(e);
>>>>> +        }
>>>>>      }
>>>>
>>>>
>>>> Maybe I don't understand the purpose of catching "NullArgumentException"
>>>> and
>>>> rethrowing something else.
>
>
> I agree and did not really liked this. It's clearly a false move by me,
> sorryr for that.
>
>
>>>>
>>>> Anyway, I was going to start a new thread about "NullArgumentException":
>>>> my
>>>> proposal is to never check for null and let the standard NPE be thrown
>>>> in
>>>> case of bad usage (null passed where a non-null is required).
>>>>
>>>> That would avoid such catch and rethrow for things that never happen.
>
>
> I think it would be better. Our own NullArgumentException don't brings any
> added value
> and I'll prefer to let the JVM handle this by itself.
>
> I don't remember the arguments and positions of everyone when we discussed
> it earlier.
>
>
>>>>
>>>>
>>>> Best regards,
>>>> Gilles
>>>>
>>>>> [...]
>>>>
>>>>
>> I was about to start a new thread too, but refrained to do so for lack
>> of knowledge on the history of this particular exception.
>> Check for null is unevenly enforced througout the library, which --to
>> me-- suggests we shouldn't do it at all and contend with NPE. There is
>> one potential use, though. I think we should check for null when the
>> NPE might occur in a different method.
>>
>> This is what happens with new Incrementor(int,
>> MaxCountExceededCallback) : cb is just copied, and fields of cb are
>> invoked elsewhere: in that case, checking for null does make sense.
>
>
> I think even there we could rely on the JVM, for simplicity.
>
Do you mean that we should do nothing, and let NPE occur "naturally"?
The origin of the problem might then be difficult to identify.
Or maybe you meant that we check for null in that case, but throw NPE
instead of our own MathNullArgument?
I would be in favor of the second option. On the other hand it's
difficult to check that it's consistently applied throughout our code,
and I can see your point in doing nothing.

Sébastien


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


Mime
View raw message