You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Simon Pepping <sp...@leverkruid.eu> on 2011/02/22 08:24:44 UTC
Re: Solving FindBugs issues
Not all FOP developers are willing to use findbugs. I hid the findbugs
errors as a courtesy to those FOP developers who do use findbugs, so
they can check their own code based on a clean slate.
FOP's history has left us with a very large number of existing
findbugs errors. It makes no sense to comment on that; it is a fact of
life. FOP's code first of all does a good job at being a successful,
heavily used FO processor. Only after that comes the question of a
clean code base.
That said, of course every FOP developer and every FOP user is welcome
to evaluate and possibly remedy one or more findbugs errors. For
precisely that reason I put comments in the exclusion file, so one can
see which errors have been simply hidden and which have been truely
evaluated.
Simon
On Mon, Feb 21, 2011 at 06:15:13PM +0000, Vincent Hennebert wrote:
> Hi,
>
> If we solve issues raised by FindBugs by listing them in an ignore file,
> is there still a point to use FindBugs at all?
>
> It seems to me that some of those issues deserve to be fixed. They seem
> to point out genuine problems in the code.
>
> Vincent
>
>
> On 18/02/11 08:18, spepping@apache.org wrote:
> > Author: spepping
> > Date: Fri Feb 18 08:18:04 2011
> > New Revision: 1071912
> >
> > URL: http://svn.apache.org/viewvc?rev=1071912&view=rev
> > Log:
> > Fixing checkstyle errors and hiding fingbugs errors
> >
> > Modified:
> > xmlgraphics/fop/trunk/findbugs-exclude.xml
> > xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java
> >
> > Modified: xmlgraphics/fop/trunk/findbugs-exclude.xml
> > URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/findbugs-exclude.xml?rev=1071912&r1=1071911&r2=1071912&view=diff
Re: Solving FindBugs issues
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Simon,
On 23/02/11 07:51, Simon Pepping wrote:
> On Tue, Feb 22, 2011 at 07:15:17PM +0000, Vincent Hennebert wrote:
>> On 22/02/11 07:24, Simon Pepping wrote:
>>> Not all FOP developers are willing to use findbugs. I hid the findbugs
>>> errors as a courtesy to those FOP developers who do use findbugs, so
>>> they can check their own code based on a clean slate.
>>
>> I agree that ignoring all the existing issues at the time FindBugs was
>> set up was necessary, but now that FindBugs is in place I don’t think we
>> want to blindly ignore its output any more. Again, some of the issues
>> raised after the recent commits seem valid and ought to be fixed.
>>
>> Can we revert commit 1071912 and re-consider the issues one-by-one
>> before ignoring them?
>
> Yes, you can.
>
> My position is as follows: I do merging of trunk into complexscripts
> as a service to the Complex Scripts project. I see existing findbugs
> errors and warnings. I do not have time nor interest to fix those. So
> I hide them so that others can run findbugs on their code from a clean
> slate. I add a new, appropriately labeled section to the findbugs
> exclusion file so that you and others can do as you suggest.
>
> I will do this again and again, no matter how many emails are written
> about the subject. And I would prefer it if no emails about it were
> written. They bother me and they discourage me to continue doing any
> services for FOP.
Are you bothered because you think the value of your services isn’t
fully recognized? I think nobody here would question your contributions
and it’s great to have you on board. I think what we’re discussing here
is just the approach we want to take regarding FindBugs, that hasn’t
been fully defined yet.
May I suggest you to not bother about FindBugs at all? Now that you’ve
set up the initial exclude file, it produces useful output that isn’t
lost among the many warnings and errors coming from the legacy code
base. I think that from now on we want to encourage committers to run
FindBugs prior to committing code, fix new issues raised by it, or make
an informed decision to ignore them.
> It is much more encouraging to see that some of us
> pick up the findbugs error report and use it to make code
> improvements.
Well it’s certainly great to see dedicated committers fixing FindBugs
issues that don’t originate from their work. For legacy code this is
what we will have to do as we go along. For new code however, I find
that hugely unfair. I think every committer should take the
responsibility to check their own code. I see this as common courtesy
towards other committers and contributors.
Do we all agree to run FindBugs along with JUnit prior to every commit
from now on?
Thanks,
Vincent
> Simon
>
>>> FOP's history has left us with a very large number of existing
>>> findbugs errors. It makes no sense to comment on that; it is a fact of
>>> life. FOP's code first of all does a good job at being a successful,
>>> heavily used FO processor. Only after that comes the question of a
>>> clean code base.
>>>
>>> That said, of course every FOP developer and every FOP user is welcome
>>> to evaluate and possibly remedy one or more findbugs errors. For
>>> precisely that reason I put comments in the exclusion file, so one can
>>> see which errors have been simply hidden and which have been truely
>>> evaluated.
>>>
>>> Simon
>>
>> Thanks,
>> Vincent
>>
>>
>>> On Mon, Feb 21, 2011 at 06:15:13PM +0000, Vincent Hennebert wrote:
>>>> Hi,
>>>>
>>>> If we solve issues raised by FindBugs by listing them in an ignore file,
>>>> is there still a point to use FindBugs at all?
>>>>
>>>> It seems to me that some of those issues deserve to be fixed. They seem
>>>> to point out genuine problems in the code.
>>>>
>>>> Vincent
>>>>
>>>>
>>>> On 18/02/11 08:18, spepping@apache.org wrote:
>>>>> Author: spepping
>>>>> Date: Fri Feb 18 08:18:04 2011
>>>>> New Revision: 1071912
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1071912&view=rev
>>>>> Log:
>>>>> Fixing checkstyle errors and hiding fingbugs errors
>>>>>
>>>>> Modified:
>>>>> xmlgraphics/fop/trunk/findbugs-exclude.xml
>>>>> xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java
>>>>>
>>>>> Modified: xmlgraphics/fop/trunk/findbugs-exclude.xml
>>>>> URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/findbugs-exclude.xml?rev=1071912&r1=1071911&r2=1071912&view=diff
Re: Solving FindBugs issues
Posted by Simon Pepping <sp...@leverkruid.eu>.
On Tue, Feb 22, 2011 at 07:15:17PM +0000, Vincent Hennebert wrote:
> On 22/02/11 07:24, Simon Pepping wrote:
> > Not all FOP developers are willing to use findbugs. I hid the findbugs
> > errors as a courtesy to those FOP developers who do use findbugs, so
> > they can check their own code based on a clean slate.
>
> I agree that ignoring all the existing issues at the time FindBugs was
> set up was necessary, but now that FindBugs is in place I don’t think we
> want to blindly ignore its output any more. Again, some of the issues
> raised after the recent commits seem valid and ought to be fixed.
>
> Can we revert commit 1071912 and re-consider the issues one-by-one
> before ignoring them?
Yes, you can.
My position is as follows: I do merging of trunk into complexscripts
as a service to the Complex Scripts project. I see existing findbugs
errors and warnings. I do not have time nor interest to fix those. So
I hide them so that others can run findbugs on their code from a clean
slate. I add a new, appropriately labeled section to the findbugs
exclusion file so that you and others can do as you suggest.
I will do this again and again, no matter how many emails are written
about the subject. And I would prefer it if no emails about it were
written. They bother me and they discourage me to continue doing any
services for FOP. It is much more encouraging to see that some of us
pick up the findbugs error report and use it to make code
improvements.
Simon
> > FOP's history has left us with a very large number of existing
> > findbugs errors. It makes no sense to comment on that; it is a fact of
> > life. FOP's code first of all does a good job at being a successful,
> > heavily used FO processor. Only after that comes the question of a
> > clean code base.
> >
> > That said, of course every FOP developer and every FOP user is welcome
> > to evaluate and possibly remedy one or more findbugs errors. For
> > precisely that reason I put comments in the exclusion file, so one can
> > see which errors have been simply hidden and which have been truely
> > evaluated.
> >
> > Simon
>
> Thanks,
> Vincent
>
>
> > On Mon, Feb 21, 2011 at 06:15:13PM +0000, Vincent Hennebert wrote:
> >> Hi,
> >>
> >> If we solve issues raised by FindBugs by listing them in an ignore file,
> >> is there still a point to use FindBugs at all?
> >>
> >> It seems to me that some of those issues deserve to be fixed. They seem
> >> to point out genuine problems in the code.
> >>
> >> Vincent
> >>
> >>
> >> On 18/02/11 08:18, spepping@apache.org wrote:
> >>> Author: spepping
> >>> Date: Fri Feb 18 08:18:04 2011
> >>> New Revision: 1071912
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1071912&view=rev
> >>> Log:
> >>> Fixing checkstyle errors and hiding fingbugs errors
> >>>
> >>> Modified:
> >>> xmlgraphics/fop/trunk/findbugs-exclude.xml
> >>> xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java
> >>>
> >>> Modified: xmlgraphics/fop/trunk/findbugs-exclude.xml
> >>> URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/findbugs-exclude.xml?rev=1071912&r1=1071911&r2=1071912&view=diff
Re: Solving FindBugs issues
Posted by Andreas Delmelle <an...@telenet.be>.
On 22 Feb 2011, at 21:50, Andreas Delmelle wrote:
> ColorUtil seems to be somewhat of a can of worms...
<snip />
> If I judge correctly, for some (if not most) of the methods, there seems to be no need for a try-catch block (?)
Sorry, forgot RuntimeExceptions, of course, which are converted into PropertyExceptions by said catch-blocks...
Even then, explicitly catching RuntimeException as a last resort would seem preferable to plain Exception. That would have avoided my confusion here, at least. ;-)
Regards,
Andreas
---
Re: Solving FindBugs issues
Posted by Andreas Delmelle <an...@telenet.be>.
On 22 Feb 2011, at 20:39, Andreas Delmelle wrote:
> ...
> --and so, I caved in started already...
In PDFFactory, I have some conflicts to work out first, but the fix-up is rather simple there. In the respective methods, eliminate all unnecessary boxing and use explicit (double) casts where appropriate.
ColorUtil seems to be somewhat of a can of worms... It's not restricted to the method where the issue was raised, but likely the result of copy-pasting a pattern from a method that was already 'excused'.
At any rate, catching plain Exception should really be avoided, IMO, and traded for catching the specific type(s) of exception we know can be thrown in that block of code. In that case, it is completely unnecessary to resort to silly-looking constructs like catching PropertyException just to rethrow it (since the method allows it anyway).
If I judge correctly, for some (if not most) of the methods, there seems to be no need for a try-catch block (?)
3 remaining on my end ;-)
Regards,
Andreas
---
Re: Solving FindBugs issues
Posted by Andreas Delmelle <an...@telenet.be>.
On 22 Feb 2011, at 20:15, Vincent Hennebert wrote:
> <snip />
> Can we revert commit 1071912 and re-consider the issues one-by-one
> before ignoring them?
+1
As far as I can see, the raised warnings are really not so challenging that they cannot be addressed right away. Implementing an equals() method, removing unread fields and redundant checks...
I'm thinking, all a matter of minutes if you put your mind to it, no?
--and so, I caved in started already...
Regards,
Andreas
---
Re: Solving FindBugs issues
Posted by Vincent Hennebert <vh...@gmail.com>.
On 22/02/11 07:24, Simon Pepping wrote:
> Not all FOP developers are willing to use findbugs. I hid the findbugs
> errors as a courtesy to those FOP developers who do use findbugs, so
> they can check their own code based on a clean slate.
I agree that ignoring all the existing issues at the time FindBugs was
set up was necessary, but now that FindBugs is in place I don’t think we
want to blindly ignore its output any more. Again, some of the issues
raised after the recent commits seem valid and ought to be fixed.
Can we revert commit 1071912 and re-consider the issues one-by-one
before ignoring them?
> FOP's history has left us with a very large number of existing
> findbugs errors. It makes no sense to comment on that; it is a fact of
> life. FOP's code first of all does a good job at being a successful,
> heavily used FO processor. Only after that comes the question of a
> clean code base.
>
> That said, of course every FOP developer and every FOP user is welcome
> to evaluate and possibly remedy one or more findbugs errors. For
> precisely that reason I put comments in the exclusion file, so one can
> see which errors have been simply hidden and which have been truely
> evaluated.
>
> Simon
Thanks,
Vincent
> On Mon, Feb 21, 2011 at 06:15:13PM +0000, Vincent Hennebert wrote:
>> Hi,
>>
>> If we solve issues raised by FindBugs by listing them in an ignore file,
>> is there still a point to use FindBugs at all?
>>
>> It seems to me that some of those issues deserve to be fixed. They seem
>> to point out genuine problems in the code.
>>
>> Vincent
>>
>>
>> On 18/02/11 08:18, spepping@apache.org wrote:
>>> Author: spepping
>>> Date: Fri Feb 18 08:18:04 2011
>>> New Revision: 1071912
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1071912&view=rev
>>> Log:
>>> Fixing checkstyle errors and hiding fingbugs errors
>>>
>>> Modified:
>>> xmlgraphics/fop/trunk/findbugs-exclude.xml
>>> xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java
>>>
>>> Modified: xmlgraphics/fop/trunk/findbugs-exclude.xml
>>> URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/findbugs-exclude.xml?rev=1071912&r1=1071911&r2=1071912&view=diff