harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Geir Magnusson Jr." <g...@pobox.com>
Subject Re: [classlib] FindBugs results for luni (HARMONY-2811)
Date Wed, 20 Dec 2006 17:34:34 GMT
Ok - I just want to be sure that it's digestible...

geir

Alexey Petrenko wrote:
> The patch is really simple. It was easy for me to review it.
> I'm just aware of few hunks...
> 
> SY, Alexey
> 
> 2006/12/20, Sian January <sianjanuary@googlemail.com>:
>> Hi Geir,
>>
>> The patch is mostly 1-line changes to different source files, which is 
>> why
>> it's so big.  It's not actually 47K of new code.  Also about half of 
>> it is
>> the same change to all the Locale classes.  However even without the 
>> Locale
>> changes the patch covers about 30 classes so I thought it might be easier
>> for people to look at one patch rather than 30.  However if you would 
>> still
>> like me to break it down I will try and see if I can break it down by
>> package or by type of bug or something like that.
>>
>> Regards,
>>
>> Sian
>>
>>
>>
>> On 20/12/06, Geir Magnusson Jr. <geir@pobox.com> wrote:
>> >
>> > 47K?
>> >
>> > Can we have the patch in digestible pieces?
>> >
>> > geir
>> >
>> >
>> > Alexey Petrenko wrote:
>> > > Guys,
>> > >
>> > > Sian January made a huge job of fixing FindBugs warnings for luni
>> > > module. The resulting patch size is 47K!
>> > > You can find the patch attached to HARMONY-2811 [1].
>> > >
>> > > I agree with the most of changes with small exceptions and need luni
>> > > experts advise.
>> > >
>> > > Index:
>> > > 
>> luni/src/main/java/org/apache/harmony/luni/platform/AdapterManager.java
>> > > ============================================================@@ -35,7
>> > > +35,7 @@
>> > >  private final HashMap<Class, List<IAdapterFactory>> factories
= new
>> > > HashMap<Class, List<IAdapterFactory>>();
>> > >
>> > >  public Object getAdapter(IAdaptable adaptable, Class adapterType) {
>> > > - List factoryList = factories.get(adaptable);
>> > > + List factoryList = factories.get(adapterType);
>> > >  if (factoryList != null) {
>> > >  for (Iterator factoryItr = factoryList.iterator(); factoryItr
>> > >  .hasNext();) {
>> > > ============================================================
>> > > FindBugs reports here that the code passes the parameter with the
>> > > wrong type to get method.
>> > > Can someone say is this substitution OK?
>> > >
>> > > Index:
>> > > 
>> luni/src/main/java/org/apache/harmony/luni/platform/DebugMemorySpy.java
>> > > ===========================================================
>> > > ---
>> > luni/src/main/java/org/apache/harmony/luni/platform/DebugMemorySpy.java
>> > > (revision 486101)
>> > > +++
>> > luni/src/main/java/org/apache/harmony/luni/platform/DebugMemorySpy.java
>> > > (working copy)
>> > > @@ -25,7 +25,7 @@
>> > >  */
>> > > final class DebugMemorySpy extends AbstractMemorySpy {
>> > >
>> > > - private final boolean stackDump = true;
>> > > + private static final boolean stackDump = true;
>> > > ============================================================FindBugs
>> > > reports that stackDump is not used. I suggest to simply remove it.
>> > >
>> > > There are also few array clonings like the following:
>> > > @@ -485,7 +485,7 @@
>> > >      * @return java.net.URL[]
>> > >      */
>> > >     public URL[] getURLs() {
>> > > -        return orgUrls;
>> > > +        return orgUrls.clone();
>> > >     }
>> > > This will definitely increase the code stability but could cause
>> > > performance degradation.
>> > > Should we accept or decline all such cases or should discuss them 
>> one by
>> > > one.
>> > > I would accept such changes.
>> > >
>> > > Thoughts?
>> > >
>> > > BTW the patch passes all the unit tests.
>> > >
>> > > SY, Alexey
>> > >
>> > > P.S. I'll apply the patch without first mentioned hunk to not to 
>> loose
>> > > the code compatibility with the huge patch.
>> >
>>
>>
>>
>> -- 
>> Sian January
>>
>> IBM Java Technology Centre, UK
>>
>>

Mime
View raw message