JDK 8 code review request for 6380161 (reflect) Exception from newInstance() not chained to cause.

[Sebastian Sickelmann]

I have picked my first Exception in this project.

And unfortunatly i picked InternalError which seems to be used quite 
often. :-)
I tried to change all initCause calls to the new Constructor and found 
many places where there is code like this:

     throw new InternalError(e.toString());
     or
     throw new InternalError(e.getMessage());
     or
     throw new InternalError("Lorem ipsum:"+e);

These ones are candidates for cleanup too, but i want to discuss if 
someone sees a good argument to not to change to
     throw new InternalError(e.getMessage(),e);
     or
     throw new InternalError("Lorem ipsum",e);

Also i don't completely understand why Throwable(Throwable cause) uses 
cause.toString() to fill the message instead of cause.getMessage().
Can someone explain to me why?

Couldn't it be
         if (cause == null) {
             detailMessage = null;
         }else {
             String message = cause.getMessage();
             detailMessage = (message==null ? null : cause.toString());
         }

instead?

- Sebastian


Am 08.08.2011 21:18, schrieb Sebastian Sickelmann:
> Thanks for encouraging me.
>
> I opened a bug [1] at bugs.openjdk.java.net. Hope that is a right 
> first step to getting started.
> I wanted to investigate the calls to initCause in jdk-source first to 
> pick the most valueable Exceptions first.
> I thinks there are many exceptions out of scope of core-libs. How to 
> handle these ones. I think i will
> start with exceptions from core-libs.
> Is there a pattern that says me exceptions belongs to which 
> openjdk-project?
>
> - Sebastian
>
> [1] https://bugs.openjdk.java.net/show_bug.cgi?id=100201
>
> Am 08.08.2011 19:04, schrieb Joe Darcy:
>> Hello.
>>
>> On 8/8/2011 7:24 AM, Sebastian Sickelmann wrote:
>>> Looks good to me.
>>>
>>> But i have one question:
>>>
>>>     Why there are two ways to plumb the causes of an exception?
>>
>> The history of this situation is covered in older versions of the 
>> javadoc of Throwable:
>> http://download.oracle.com/javase/6/docs/api/java/lang/Throwable.html
>>
>>>     In many exceptions-classes you can use a constuctor-level 
>>> argument to specify the cause, and in some classes you must use the 
>>> initCause method.
>>>
>>>     Is it: "When you often need a cause at creation-time than we got 
>>> a specializes constructor." ?
>>>
>>>     I found 500+ lines jdk/src/share/classes/**/*.java where i found 
>>> line like
>>>
>>>            XYZException iae = new XYZException();
>>>             iae.initCause(e);
>>>             throw iae;
>>>
>>>     I am searching a minor project for my second Contribution to 
>>> OpenJDK to learn more about the developement / review process.
>>>     Is it worthwhile to look at this issue and maybe refactor some 
>>> of the often used Exceptions to accept a cause at construction-time?
>>
>> I would say such a project would be helpful; for context, this topic 
>> came up a few months ago on core-libs in the context of another code 
>> review:
>>
>>     
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-June/007005.html
>>     
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-June/007008.html
>>
>> Cheers,
>>
>> -Joe
>>
>>>
>>> - Sebastian
>>>
>>> Am 08.08.2011 07:24, schrieb Joe Darcy:
>>>> Hello.
>>>>
>>>> Please review this small fix, developed after I went through my 
>>>> open bug list; patch below, webrev at:
>>>>
>>>>    6380161 (reflect) Exception from newInstance() not chained to 
>>>> cause.
>>>>    http://cr.openjdk.java.net/~darcy/6380161.0/
>>>>
>>>> Several call to initCause are added to plumb up caused-by exception 
>>>> chains to exception types without constructors taking a cause.  In 
>>>> addition, multi-catch is added in on location.
>>>>
>>>> I looked for similar issues in Constructor, Method, and Executable, 
>>>> but there weren't any.
>>>>
>>>> Thanks,
>>>>
>>>> -Joe
>>>>
>>>> --- old/src/share/classes/java/lang/Class.java    2011-08-07 
>>>> 22:21:58.000000000 -0700
>>>> +++ new/src/share/classes/java/lang/Class.java    2011-08-07 
>>>> 22:21:58.000000000 -0700
>>>> @@ -349,7 +349,8 @@
>>>>                         });
>>>>                 cachedConstructor = c;
>>>>             } catch (NoSuchMethodException e) {
>>>> -                throw new InstantiationException(getName());
>>>> +                throw (InstantiationException)
>>>> +                    new 
>>>> InstantiationException(getName()).initCause(e);
>>>>             }
>>>>         }
>>>>         Constructor<T> tmpConstructor = cachedConstructor;
>>>> @@ -973,7 +974,8 @@
>>>>                 descriptor      = (String)   enclosingInfo[2];
>>>>                 assert((name != null && descriptor != null) || name 
>>>> == descriptor);
>>>>             } catch (ClassCastException cce) {
>>>> -                throw new InternalError("Invalid type in enclosing 
>>>> method information");
>>>> +                throw (InternalError)
>>>> +                    new InternalError("Invalid type in enclosing 
>>>> method information").initCause(cce);
>>>>             }
>>>>         }
>>>>
>>>> @@ -1239,7 +1241,8 @@
>>>>         try {
>>>>             return 
>>>> getName().substring(enclosingClass.getName().length());
>>>>         } catch (IndexOutOfBoundsException ex) {
>>>> -            throw new InternalError("Malformed class name");
>>>> +            throw (InternalError)
>>>> +                new InternalError("Malformed class 
>>>> name").initCause(ex);
>>>>         }
>>>>     }
>>>>
>>>> @@ -2954,9 +2957,8 @@
>>>>             }
>>>>             // These can happen when users concoct enum-like classes
>>>>             // that don't comply with the enum spec.
>>>> -            catch (InvocationTargetException ex) { return null; }
>>>> -            catch (NoSuchMethodException ex) { return null; }
>>>> -            catch (IllegalAccessException ex) { return null; }
>>>> +            catch (InvocationTargetException | 
>>>> NoSuchMethodException |
>>>> +                   IllegalAccessException ex) { return null; }
>>>>         }
>>>>         return enumConstants;
>>>>     }
>>>>
>>>
>>
>