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