You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Alexey Petrenko <al...@gmail.com> on 2006/12/20 17:05:43 UTC
[classlib] FindBugs results for luni (HARMONY-2811)
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.
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
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 <si...@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. <ge...@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
>>
>>
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by Alexey Petrenko <al...@gmail.com>.
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 <si...@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. <ge...@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
>
>
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by Sian January <si...@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. <ge...@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
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
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.
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by Alexey Varlamov <al...@gmail.com>.
>
> 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.
AFAIU this particular fix concerns j.l.URLClassLoader, so this might
be security hole as well. Anyway, let's first get code right, then
tune if needed.
> 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.
>
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by Sian January <si...@googlemail.com>.
On 21/12/06, Alexey Petrenko <al...@gmail.com> wrote:
>
> 2006/12/21, Tim Ellison <t....@gmail.com>:
> > Alexey Petrenko wrote:
> > > Ok, since nobody objects...
> >
> > Phew, you didn't give much time to look at it -- I got your 'please
> > look' and 'nobody objects' messages 18hrs apart.
> >
> > I'll look at the commit log and will raise any issues I have.
> >
> > > I'll apply the patch almost as is. I agree with Sian on his comment
> >
> > Just FYI:
> > s/his/her/
> Ooops... 8-)
> Sorry, Sian. :)
No problem :)
SY, Alexey
>
--
Sian January
IBM Java Technology Centre, UK
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by Alexey Petrenko <al...@gmail.com>.
2006/12/21, Tim Ellison <t....@gmail.com>:
> Alexey Petrenko wrote:
> > Ok, since nobody objects...
>
> Phew, you didn't give much time to look at it -- I got your 'please
> look' and 'nobody objects' messages 18hrs apart.
>
> I'll look at the commit log and will raise any issues I have.
>
> > I'll apply the patch almost as is. I agree with Sian on his comment
>
> Just FYI:
> s/his/her/
Ooops... 8-)
Sorry, Sian. :)
SY, Alexey
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by Tim Ellison <t....@gmail.com>.
Alexey Petrenko wrote:
> Ok, since nobody objects...
Phew, you didn't give much time to look at it -- I got your 'please
look' and 'nobody objects' messages 18hrs apart.
I'll look at the commit log and will raise any issues I have.
> I'll apply the patch almost as is. I agree with Sian on his comment
Just FYI:
s/his/her/
Regards,
Tim
> about AdapterManager modification.
> But I'll remove DebugMemorySpy update.
>
> SY, Alexey
Re: [classlib] FindBugs results for luni (HARMONY-2811)
Posted by Alexey Petrenko <al...@gmail.com>.
Ok, since nobody objects...
I'll apply the patch almost as is. I agree with Sian on his comment
about AdapterManager modification.
But I'll remove DebugMemorySpy update.
SY, Alexey
2006/12/20, Alexey Petrenko <al...@gmail.com>:
> 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.
>