On 16/06/14 23:32, Glenn Adams wrote:
By the same logic, checkstyle is a huge waste of time and effort.


I find this quite amusing to read given that I am the one who promoted
the setting up of Checkstyle in CI, submitted several revisions of
a Checkstyle configuration file that would reach the agreement of
everybody, then spent hours making the code base compliant...


As is
paying attention to deprecations.

If you have failed to see findbugs find a bug worth fixing, then it is
because you refuse to use it and don't pay attention to its output. I have
seen at least a half a dozen real bugs discovered by findbugs on FOP, and
I'm sure there are likely others being masked by the current excludes file
which bear further investigation.

It’s certainly a shame that this valuable feedback is lost in such
a high amount of noise. Within the past week working on a private
project, I had a dozen or so of warnings and all of them were pure noise
that I had to ignore in the exclude file.


I'm going to enable it for nightly builds, and hopefully you will join the
rest of us in using it.

If by this you mean change the Gump descriptor to make CI fail on
FindBugs warnings, then please first submit an exclude file that
everybody would agree on. Consensus has to be reached before taking
action on this.


Thanks,
Vincent


On Mon, Jun 16, 2014 at 10:02 AM, Vincent Hennebert
<vhenneb...@gmail.com>
wrote:

On 16/06/14 16:10, Glenn Adams wrote:

Given that, with the exception of yourself, the findbugs exclude file has
been continually updated to produce no warnings, I conclude that the FOP
dev ha implicitly or explicitly accepted it for quite some time.


And yet, consensus has not been reached on this, since at least one
developer is not keen on using it.

If there were a real benefit in using FindBugs, then I would be the
first one to promote its use. But that’s not the case, at least not in
the present state.

Quite on the contrary, I’ve seen bad things done to the code for the
sake of keeping FindBugs quiet. Like surrounding whole chunks of code
with checks for null when the variables are actually not meant to be
null. Or instanceof checks when the variable can have only one type.

Such checks seriously impede the readability of the code. Whoever comes
after that will wonder why there is a check for null, and why nothing is
done when the variable is null.

And if that were only that. They also make debugging much harder by
silently ignoring errors instead of making the code fail-fast.

I have yet to see cases where I think: “Oh yes that’s right, there was
a serious bug here, thanks FindBugs for letting me know.” In most cases,
it’s rather: “Naah, that’s not an error, I’m going to have to update
this exclude file again, which pattern should I put again, where’s the
page that lists them, ah here it is, oh dear, which one is it among
those hundreds?” And that’s to remain polite :-)



  We don't need to do anything to the excludes file to make it acceptable.
It
is acceptable now. That doesn't mean it couldn't be improved by additional
review and revision, but that is true of the entire FOP code base.


I hope the above shows why it’s not acceptable as it is. At the moment
and in my opinion, running FindBugs is a waste of time and energy rather
than a help.


Vincent



  On Mon, Jun 16, 2014 at 3:32 AM, Vincent Hennebert
<vhenneb...@gmail.com>
wrote:

  Rather than repeating myself, I’ll just point to the past conversations
on this topic:
http://markmail.org/message/pg7p2khityg4bjya
http://markmail.org/message/taztvj2ms7x6tryb

This one is particularly interesting:
http://markmail.org/message/lthcstbbmhtbrsva
It more or less matches my own experience with FindBugs.

The only way to ensure zero FindBugs warnings is to enforce it in
continuous integration, just like we did about Checkstyle. But before
doing that, you’ll have to come up with a satisfying exclude file that
filters out uninteresting warnings, and get the community to agree on
it.

Vincent



On 15/06/14 17:03, Glenn Adams wrote:

  I'm getting tired of fixing findbugs warnings that people are leaving in
the
code base. I'm going to start inserting comments that will break
compiles/builds if people don't check and fix these. Today I see four
new
warnings:

*Dodgy Warnings*

*Code*



*Warning*

*BC*




Unchecked/unconfirmed cast from org.apache.fop.fo.FONode to
org.apache.fop.fo.flow.MultiCase in
org.apache.fop.layoutmgr.LayoutManagerMapping$
MultiCaseLayoutManagerMaker.make(FONode,
List)




*BC*




Unchecked/unconfirmed cast from org.apache.fop.fo.FONode to
org.apache.fop.fo.flow.MultiSwitch in
org.apache.fop.layoutmgr.LayoutManagerMapping$
MultiSwitchLayoutManagerMaker.make(FONode,
List)





*BC*




Unchecked/unconfirmed cast from org.apache.fop.fo.FObj to
org.apache.fop.fo.flow.MultiSwitch in new
org.apache.fop.layoutmgr.MultiSwitchLayoutManager(FObj)





*RCN*




Redundant nullcheck of footnoteList, which is known to be non-null in
org.apache.fop.layoutmgr.list.ListItemLayoutManager.
getCombinedKnuthElementsForListItem(List,
List, LayoutContext)









Reply via email to