On Tue, Jun 17, 2014 at 1:36 AM, Vincent Hennebert <vhenneb...@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
>> <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