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 Glenn Adams <gl...@skynav.com> on 2014/06/15 17:03:01 UTC

Findbugs Again!

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)

Re: Findbugs Again!

Posted by Glenn Adams <gl...@skynav.com>.

On Tue, Jun 17, 2014 at 1:36 AM, Vincent Hennebert <vh...@gmail.com>
wrote:

> 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.
>

We have been using the same (growing) exclude file for a number of years.
It represents a consensus due to its existence and use. If you wish to
change it, feel free to do so, provided findbugs produces no warnings.


>
>
> Thanks,
>
> Vincent
>
>
>  On Mon, Jun 16, 2014 at 10:02 AM, Vincent Hennebert
>> <vh...@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
>>>
>>>> <vh...@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)
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>

Re: Findbugs Again!

Posted by Glenn Adams <gl...@skynav.com>.

On Tue, Jun 17, 2014 at 1:36 AM, Vincent Hennebert <vh...@gmail.com>
wrote:

> 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...


my irony was intentional: you spent an exceeding amount of time on
checkstyle which cannot by definition find *any* bug, but you are now
complaining that findbugs may not find a worthy bugs and use that to argue
against findbugs...



>
>
>
>  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
>> <vh...@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
>>>
>>>> <vh...@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)
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>

Re: Findbugs Again!

Posted by Vincent Hennebert <vh...@gmail.com>.

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
> <vh...@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
>>> <vh...@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)
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>

Re: Findbugs Again!

Posted by Glenn Adams <gl...@skynav.com>.

By the same logic, checkstyle is a huge waste of time and effort. 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.

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



On Mon, Jun 16, 2014 at 10:02 AM, Vincent Hennebert <vh...@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
>> <vh...@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)
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>

Re: Findbugs Again!

Posted by Vincent Hennebert <vh...@gmail.com>.

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
> <vh...@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)
>>>
>>>
>>>
>>>
>>>
>>>
>

Re: Findbugs Again!

Posted by Glenn Adams <gl...@skynav.com>.

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.

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.



On Mon, Jun 16, 2014 at 3:32 AM, Vincent Hennebert <vh...@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)
>>
>>
>>
>>
>>
>>

Re: Findbugs Again!

Posted by Vincent Hennebert <vh...@gmail.com>.

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)
>
>
> 	
>
>