On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <[email protected]> wrote:
> > On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <[email protected]>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. > > >> >> Thanks, >> Vincent >> > >
