On Fri, Nov 1, 2013 at 9:11 AM, Vincent Hennebert <vhenneb...@gmail.com>wrote:

> On 01/11/13 16:52, Glenn Adams wrote:
>
>> On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <gl...@skynav.com> wrote:
>>
>>
>>> On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <vhenneb...@gmail.com
>>> >wrote:
>>>
>>>  We have here a typical example of how code is being made worse due to
>>>> the rigidity of a code checking tool:
>>>>
>>>>   Author: gadams
>>>>
>>>>> Date: Thu Oct 31 19:44:25 2013
>>>>> New Revision: 1537600
>>>>>
>>>>> Modified:
>>>>> xmlgraphics/fop/trunk/src/****java/org/apache/fop/render/**
>>>>> pdf/PDFStructureTreeBuilder.****java
>>>>> URL: 
>>>>> http://svn.apache.org/viewvc/****xmlgraphics/fop/trunk/src/**<http://svn.apache.org/viewvc/**xmlgraphics/fop/trunk/src/**>
>>>>> java/org/apache/fop/render/****pdf/PDFStructureTreeBuilder.**
>>>>> java?rev=1537600&r1=1537599&****r2=1537600&view=diff<http://**
>>>>> svn.apache.org/viewvc/**xmlgraphics/fop/trunk/src/**
>>>>> java/org/apache/fop/render/**pdf/PDFStructureTreeBuilder.**
>>>>> java?rev=1537600&r1=1537599&**r2=1537600&view=diff<http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java?rev=1537600&r1=1537599&r2=1537600&view=diff>
>>>>> >
>>>>> ==============================****============================**==**
>>>>> ==================
>>>>> --- xmlgraphics/fop/trunk/src/****java/org/apache/fop/render/**
>>>>> pdf/PDFStructureTreeBuilder.****java (original)
>>>>> +++ xmlgraphics/fop/trunk/src/****java/org/apache/fop/render/**
>>>>> pdf/PDFStructureTreeBuilder.****java Thu Oct 31 19:44:25 2013
>>>>>
>>>>> @@ -358,7 +358,12 @@ class PDFStructureTreeBuilder implements
>>>>>        }
>>>>>
>>>>>        public StructureTreeElement startNode(String name, Attributes
>>>>> attributes, StructureTreeElement parent) {
>>>>> -        PDFStructElem parentElem = parent == null ?
>>>>> ancestors.getFirst() : (PDFStructElem) parent;
>>>>> +        PDFStructElem parentElem;
>>>>> +        if ((parent != null) && (parent instanceof PDFStructElem)) {
>>>>> +            parentElem = (PDFStructElem) parent;
>>>>> +        } else {
>>>>> +            parentElem = ancestors.getFirst();
>>>>> +        }
>>>>>            PDFStructElem structElem = createStructureElement(name,
>>>>> parentElem, attributes,
>>>>>                    pdfFactory, eventBroadcaster);
>>>>>            ancestors.addFirst(structElem)****;
>>>>>
>>>>>
>>>> The modified code doesn’t behave the same as the original. In the
>>>> original code, if parent is not null but is not an instance of
>>>> PDFStructElem, then a ClassCastException will occur.
>>>>
>>>> In the new code, parentElem will silently be set to
>>>> ancestors.getFirst().
>>>>
>>>> The original code is fail-fast: if a mistake is introduced in a later
>>>> modification, such that it’s no longer an instance of PDFStructElem that
>>>> is being passed, then it will throw an exception. Forcing the developer
>>>> to reconsider the change, or adapt this method appropriately.
>>>>
>>>> With the new version, no exception will be thrown, and depending on the
>>>> modification, it may or may not be correct. More likely not. And this
>>>> will go unnoticed. And if it’s noticed, then a debugging session will
>>>> have to be launched, and the code studied, and after a while the
>>>> developer may arrive in this method, and then wonder why there is this
>>>> instanceof check, what does it mean, what should be done for
>>>> a non-PDFStructElem, etc. And after some (long) time being passed they
>>>> might figure out that there was a mistake in their modification.
>>>>
>>>> I suggest you revert this change.
>>>>
>>>>
>>> No thank you. The original code is broken since it depends on use of a
>>> ClassCastException when it shouldn't. I only fixed this because findbugs
>>> reported this problem and whomever checked in in the first place didn't
>>> fix
>>> the warning correctly.
>>>
>>> I have recently pointed out on a couple of occasions that people are
>>> leaving checkstyle or findbugs warnings in the tree and not fixing them.
>>> If
>>> they don't fix them in the first place, then I'll do it when I do a
>>> commit
>>> in order to get a clean tree. If they want to go back and revise my fix,
>>> that's fine with me as long as they don't leave warnings lying around.
>>>
>>>
>> To elaborate to make sure I'm understood:
>>
>> If anyone commits a change that introduces a new checkbug or findbug
>> warning, or breaks an existing junit test, then that person is responsible
>> for fixing the problem ASAP. If they fail to do so, then whatever fix-ups
>> get applied by other committers is acceptable provided they don't
>> introduce
>> other new warnings or break existing tests.
>>
>> This is just SOP.
>>
>
> We do have a no Checkstyle warning policy after the revised Checkstyle
> configuration file we agreed on some time ago.
>
> However, we have never made a similar agreement on FindBugs, precisely
> because people couldn’t agree on the interest of this tool due to the
> high ratio of noise (at least in its standard configuration).
>
> If you want to use FindBugs on your local copy, then you’re absolutely
> free to do so, but please don’t impose it on other people without first
> reaching an agreement about it.
>

I beg to differ. We have been operating on a no warnings policy for
findbugs as long as I've been with the project. The fact that folks are
keeping it up to date (without warnings) in general proves that is the case.


>
> IMO your change is not acceptable because it makes the code less
> future-proof (less robust to future changes). Which to me is at least as
> important as introducing warnings or breaking tests.


So fix it in the way you like that doesn't introduce a findbugs warning.


>
>
>
> Vincent
>

Reply via email to