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 [classlib] FindBugs results for luni (HARMONY-2811)
Date Wed, 20 Dec 2006 16:05:43 GMT
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