2011/2/26 Leonard Wörteler leonard.woerteler@uni-konstanz.de:
Hi Godmar,
that bug was pretty easy to fix, and I have no idea how the superfluous downcast passed all our tests. It was introduced in version 6.2.8.
Generally, downcasts are bad software engineering practice and can and should be avoided or minimized by proper design. Either change the method signature to let the compiler flag attends at passing incompatible types, or add virtual methods to a superclass/interface, or (last resort) use instanceof.
Findbugs is a tool that flags errors such as these; you may have heard of it.
Running Findbugs over the current codebase yields 563 warnings, some of which are clear bugs. I recommend you do that.
For example:
void delete(final int pre, final int size) { NSNode nd = root.find(pre); if(nd.pre == pre) nd = nd.par; if(nd == null) root = rootDummy; while(!nd.equals(rootDummy)) { nd.delete(pre, size); nd = nd.par; } }
if 'nd == null', 'nd.equals()' crashes. If it can't be null, then 'root = rootDummy' is bogus.
Or:
case GRANT: final CmdPerm perm = consume(CmdPerm.class, cmd); if(perm == null) help(null, cmd); final String db = key(ON, null) ? name(cmd) : null; key(TO, cmd); return db == null ? new Grant(perm, name(cmd)) : new Grant(perm, name(cmd), db);
if 'perm' is null, you crash in the 'Grant' constructor when calling perm.toString(). If it can't be null, the check is bogus.
Or:
@Override public boolean uses(final Use u) { return u == Use.CTX && (def == FunDef.MB || def == FunDef.MB || def == FunDef.EVAL || def == FunDef.RUN) || super.uses(u); }
Did you mean to check for two different values instead of FunDef.MB twice.
Or:
// [CG] XQuery/Formatter: calendars and places are ignored if(cal != null || plc != null);
Did you mean to return if cal != null || plc != null rather than doing nothing?
Or in FTWrods.java
if(iat == null) { final FTLexer lex = new FTLexer(ftt.opt);
/* elided */ if(iat == null) { iat = ia; tl = t; } else if(mode == FTMode.M_ALL || mode == FTMode.M_ALLWORDS) { iat = FTIndexIterator.intersect(ia, iat, 0); tl += t; } else { iat = FTIndexIterator.union(ia, iat); tl = Math.max(t, tl); } iat.tokenNum(++ctx.ftoknum); } return iat.more() ? new FTNode(iat.matches(), data, iat.next(), tl, iat.indexSize(), iat.score()) : null; }
The code in the else branch is dead - iat is always null where you test it due to the dominating test on entry. The 'intersect' and 'union' call is never made.
Etc. well worth the time fixing.
- Godmar