harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexey Petrenko" <alexey.a.petre...@gmail.com>
Subject Re: [classlib] FindBugs results for luni (HARMONY-2811)
Date Wed, 20 Dec 2006 17:19:53 GMT
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