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

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.


Vincent

Reply via email to