Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
Yes, there is are good reasons to continue using findbugs:
(1) it does find bugs
(2) they should not be added the exclude file without careful evaluation
(3) when evaluating, real bugs should be fixed; however, there are some
cases where findbugs reports a warning or error that is in fact not a bug,
but is intended to be that way by design; for example, returning a null vs
returning an empty array: findbugs will report the former as a warning, but
some designs may prefer that route;
Any exclusions added to the findbugs-exclude file should be done carefully
and after explicit evaluation.
I know that has not been the case recently, due to lack of time, and some
exclusions have been added without evaluation. This should be avoided, and
any exclusions added automatically should be marked as such for further
evaluation and repair.
G.
On Mon, Feb 21, 2011 at 11:15 AM, Vincent Hennebert vhenneb...@gmail.comwrote:
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, spepp...@apache.org wrote:
Author: spepping
Date: Fri Feb 18 08:18:04 2011
New Revision: 1071912
URL: http://svn.apache.org/viewvc?rev=1071912view=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=1071912r1=1071911r2=1071912view=diff
==
--- xmlgraphics/fop/trunk/findbugs-exclude.xml (original)
+++ xmlgraphics/fop/trunk/findbugs-exclude.xml Fri Feb 18 08:18:04 2011
@@ -5019,4 +5019,51 @@
Bug pattern=UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR/
/Match
!-- /Automatically generated list of exclusions --
+ !-- Automatically generated list of exclusions on 18 February 2011
--
+ Match
+ Class name=org.apache.fop.afp.goca.GraphicsSetProcessColor/
+ Method name=writeToStream/
+ Bug pattern=OS_OPEN_STREAM/
+ /Match
+ Match
+ Class name=org.apache.fop.pdf.PDFFactory/
+ Method name=makeSeparationColorSpace/
+ Bug pattern=DM_FP_NUMBER_CTOR/
+ /Match
+ Match
+ Class name=org.apache.fop.pdf.PDFFactory/
+ Method name=toColorVector/
+ Bug pattern=DM_FP_NUMBER_CTOR/
+ /Match
+ Match
+ Class name=org.apache.fop.fo.expr.PropertyTokenizer/
+ Field name=recognizeOperator/
+ Bug pattern=URF_UNREAD_FIELD/
+ /Match
+ Match
+ Class name=org.apache.fop.layoutmgr.BlockLayoutManager/
+ Method name=getNextChildElements/
+ Bug pattern=BC_UNCONFIRMED_CAST/
+ /Match
+ Match
+ Class name=org.apache.fop.util.ColorWithFallback/
+ !-- Listing the method 'equals' does not work --
+ Bug pattern=EQ_DOESNT_OVERRIDE_EQUALS/
+ /Match
+ Match
+ Class name=org.apache.fop.util.ColorWithFallback/
+ Method name=getAlternativeColors/
+ Bug pattern=PZLA_PREFER_ZERO_LENGTH_ARRAYS/
+ /Match
+ Match
+ Class name=org.apache.fop.fo.expr.NamedColorFunction/
+ Method name=eval/
+ Bug pattern=RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE/
+ /Match
+ Match
+ Class name=org.apache.fop.util.ColorUtil/
+ Method name=parseAsFopRgbNamedColor/
+ Bug pattern=REC_CATCH_EXCEPTION/
+ /Match
+ !-- /Automatically generated list of exclusions on 18 February 2011
--
/FindBugsFilter
\ No newline at end of file
Modified:
xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java
URL:
http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java?rev=1071912r1=1071911r2=1071912view=diff
==
---
xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java
(original)
+++
xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java
Fri Feb 18 08:18:04 2011
@@ -60,7 +60,8 @@ public class PDFImageHandlerSVG implemen
private static Log log =
LogFactory.getLog(PDFImageHandlerSVG.class);
/** {@inheritDoc} */
-public void handleImage(RenderingContext context, Image image,
Rectangle pos)
+public void handleImage(RenderingContext context,//
CSOK: MethodLength
+Image image, Rectangle pos)
throws IOException {
PDFRenderingContext pdfContext = (PDFRenderingContext)context;
PDFContentGenerator generator =
Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
On 21 Feb 2011, at 19:15, Vincent Hennebert wrote: 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. I was about to convey a similar sentiment. If we are only going to ignore potential bugs, the point of the whole exercise seems totally lost. Some of them may be OK to ignore, as Glenn pointed out, but IIUC, one of those potentially introduced bugs (that we are now ignoring) is likely of the same nature as one that Chris fixed in the very same area a while back (potentially unclosed stream, leading to a descriptor leak in the AFP renderer)... Not very encouraging. :( Mea culpa: I saw one exclusion --unconfirmed cast-- that would seem to stem from my recent refactoring in the BlockStackingLMs. Not sure why an exclusion was chosen here, but adding an assert statement in the code avoids the warning as well *and* has the benefit of being visible exactly at the spot in the code where it applies. Seemed like the more proper way to handle this. Just my 2 cents... Regards, Andreas ---
Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
On 21 Feb 2011, at 20:28, Andreas Delmelle wrote: On 21 Feb 2011, at 19:15, Vincent Hennebert wrote: 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. I was about to convey a similar sentiment. If we are only going to ignore potential bugs, the point of the whole exercise seems totally lost. Note also that, IIUC, if you define an exclusion for a given bug type in the scope of a given method, future devs are free to introduce many more instances of that very same bug type in that same method, without ever noticing that they are doing something wrong --even if they do their due diligence and run FB before each commit? That just doesn't seem right. :-/ Regards, Andreas ---
Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
First, the point of the exclusion file is to bless warnings that, *by design*, do not correspond with findbugs set of default warnings. The presumption is that findbugs actually does identify real or potential bugs, and there should be no argument about whether this is true or not. What we are discussing here is the *automatic* addition of entries to the exclusion file. In general, that is a *bad practice*, however, for expediency purposes that has been done a few times already. Such practice should not be continued. Every exclusion added to the exclude file should be accompanied by a comment in the exclude file, and perhaps a comment in the source file as well, indicating why the exclusion occurred. If folks are uncomfortable with marking an exclusion for an entire method, then they can mark the specific line of source as well; however, that is problematic when line numbers change. I feel it is better to mark it for the method even if there is a risk it will unintentionally apply to another line (in the same method) than was intended. If folks make their methods shorter, then this reduces the likelihood of an unintentional exclusion. My personal criteria is that, in the general case, a method should occupy no more than one screen (on your favorite text editor). G. On Mon, Feb 21, 2011 at 12:35 PM, Andreas Delmelle andreas.delme...@telenet.be wrote: On 21 Feb 2011, at 20:28, Andreas Delmelle wrote: On 21 Feb 2011, at 19:15, Vincent Hennebert wrote: 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. I was about to convey a similar sentiment. If we are only going to ignore potential bugs, the point of the whole exercise seems totally lost. Note also that, IIUC, if you define an exclusion for a given bug type in the scope of a given method, future devs are free to introduce many more instances of that very same bug type in that same method, without ever noticing that they are doing something wrong --even if they do their due diligence and run FB before each commit? That just doesn't seem right. :-/ Regards, Andreas ---
RE: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
I haven't looked at the trunk lately but 1.0 has a ton of warnings, at least in my compile. I don't know how much warnings have changed over the versions. I think it was originally written to compile on Java 1.4 or maybe even 1.3. 1.5 shows thousands of warnings, 1.6 shows more. Some of the warnings are quirky (raw type list?), some are just wasteful (dead code? Local variable never referenced?). If there's code which is actually incorrect logic it needs to be fixed. If there's code which is just incomplete logic, maybe setting something up for a future improvement someone wanted to add, that should be removed and placed in a separate to do list document. -Original Message- From: Andreas Delmelle [mailto:andreas.delme...@telenet.be] Sent: Monday, February 21, 2011 2:29 PM To: fop-dev@xmlgraphics.apache.org Subject: Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java] On 21 Feb 2011, at 19:15, Vincent Hennebert wrote: 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. I was about to convey a similar sentiment. If we are only going to ignore potential bugs, the point of the whole exercise seems totally lost. Some of them may be OK to ignore, as Glenn pointed out, but IIUC, one of those potentially introduced bugs (that we are now ignoring) is likely of the same nature as one that Chris fixed in the very same area a while back (potentially unclosed stream, leading to a descriptor leak in the AFP renderer)... Not very encouraging. :( Mea culpa: I saw one exclusion --unconfirmed cast-- that would seem to stem from my recent refactoring in the BlockStackingLMs. Not sure why an exclusion was chosen here, but adding an assert statement in the code avoids the warning as well *and* has the benefit of being visible exactly at the spot in the code where it applies. Seemed like the more proper way to handle this. Just my 2 cents... Regards, Andreas ---
Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
The current trunk shows no warnings during ANT compile. Please make reference to the current trunk/HEAD as 1.0 is published and history at this time. It's a different matter with certain IDEs, e.g., Eclipse, which set their warning levels to a more sensitive level than the ANT build. Although it would be nice to eliminate such additional warnings, it is not as high priority IMO as ensuring that new compile, checkstyle, or findbugs warnings/errors do not appear during ANT builds. At the same time, warnings that do appear should not automatically be excluded without careful, manual review. G. On Mon, Feb 21, 2011 at 12:49 PM, Eric Douglas edoug...@blockhouse.comwrote: I haven't looked at the trunk lately but 1.0 has a ton of warnings, at least in my compile. I don't know how much warnings have changed over the versions. I think it was originally written to compile on Java 1.4 or maybe even 1.3. 1.5 shows thousands of warnings, 1.6 shows more. Some of the warnings are quirky (raw type list?), some are just wasteful (dead code? Local variable never referenced?). If there's code which is actually incorrect logic it needs to be fixed. If there's code which is just incomplete logic, maybe setting something up for a future improvement someone wanted to add, that should be removed and placed in a separate to do list document. -Original Message- From: Andreas Delmelle [mailto:andreas.delme...@telenet.be] Sent: Monday, February 21, 2011 2:29 PM To: fop-dev@xmlgraphics.apache.org Subject: Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java] On 21 Feb 2011, at 19:15, Vincent Hennebert wrote: 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. I was about to convey a similar sentiment. If we are only going to ignore potential bugs, the point of the whole exercise seems totally lost. Some of them may be OK to ignore, as Glenn pointed out, but IIUC, one of those potentially introduced bugs (that we are now ignoring) is likely of the same nature as one that Chris fixed in the very same area a while back (potentially unclosed stream, leading to a descriptor leak in the AFP renderer)... Not very encouraging. :( Mea culpa: I saw one exclusion --unconfirmed cast-- that would seem to stem from my recent refactoring in the BlockStackingLMs. Not sure why an exclusion was chosen here, but adding an assert statement in the code avoids the warning as well *and* has the benefit of being visible exactly at the spot in the code where it applies. Seemed like the more proper way to handle this. Just my 2 cents... Regards, Andreas ---
Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
On 21 Feb 2011, at 20:49, Eric Douglas wrote: I haven't looked at the trunk lately but 1.0 has a ton of warnings, at least in my compile. I don't know how much warnings have changed over the versions. I think it was originally written to compile on Java 1.4 or maybe even 1.3. Even lower, IIRC. FOP ultimately dates back to last century. 1.5 shows thousands of warnings, 1.6 shows more. Some of the warnings are quirky (raw type list?), Those are the 'unchecked assignment' type. On the one hand, not much to worry about, as it has posed little problems in the past. On the other, it did sometimes cause nasty ClassCastExceptions... So we definitely try to eliminate those as we go. This number should reduce over time. Regards, Andreas ---
Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
On 21 Feb 2011, at 21:18, Eric Douglas wrote: I always had trouble with the ant build. Currently my ant build gets no errors and no warnings and creates fop.jar though it tells me junit and xmlunit are not found. Last time I put jars in my fop lib folder for junit and xmlunit it told me they were found and it wouldn't update the fop jar. If there's already a set of precompiled classes, and none of the sources have changed, there is no need to update the jar. If you just run 'ant compile' or 'ant package', neither JUnit nor XMLUnit are required for a successful build. Compiler warnings are not errors, and as such do not prevent a correct build. How can I get hundreds or even thousands of warnings from trying to use the eclipse build project option with the default settings while the ant build has no errors? The 'ant compile' task does show a hint of those warnings, IIC. Look for Note: some classes use unchecked or unsafe operations. If I were to guess, I'd say your IDE probably compiles with '-Xlint:unchecked', which displays all the gruesome details. Regards, Andreas ---
Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
On 21/02/11 19:28, Andreas Delmelle wrote: On 21 Feb 2011, at 19:15, Vincent Hennebert wrote: 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. I was about to convey a similar sentiment. If we are only going to ignore potential bugs, the point of the whole exercise seems totally lost. Some of them may be OK to ignore, as Glenn pointed out, but IIUC, one of those potentially introduced bugs (that we are now ignoring) is likely of the same nature as one that Chris fixed in the very same area a while back (potentially unclosed stream, leading to a descriptor leak in the AFP renderer)... Not very encouraging. :( Mea culpa: I saw one exclusion --unconfirmed cast-- that would seem to stem from my recent refactoring in the BlockStackingLMs. Not sure why an exclusion was chosen here, but adding an assert statement in the code avoids the warning as well *and* has the benefit of being visible exactly at the spot in the code where it applies. Seemed like the more proper way to handle this. Did you remove the corresponding entry from the findbugs-exclude.xml file before running FindBugs again? Just my 2 cents... Regards, Andreas Vincent
Re: Solving FindBugs issues [was: Re: svn commit: r1071912 - in /xmlgraphics/fop/trunk: findbugs-exclude.xml src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java]
On 21 Feb 2011, at 22:18, Vincent Hennebert wrote: Did you remove the corresponding entry from the findbugs-exclude.xml file before running FindBugs again? Locally, yes, and confirmed that the assert eliminated the warning. The exclusions file seems to be needing some more general cleanup, so I haven't committed that yet. 1012 exclusions is probably way too many already... :-/ Regards, Andreas ---
