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 16:24:45 GMT
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.

Mime
View raw message