Thanks all. Despite my aesthetic preference for removing unused code, I'm *really* not in favor of causing extra work (for myself or others) to satisfy it <G>.. Especially when there's reasonable expectations that the code in question *will* be used in the foreseeable future.....
Ok, I'll leave the code in place as-is and provide a patch with unit tests sometime real soon now. Erick On Thu, Apr 23, 2009 at 6:15 AM, Michael McCandless < luc...@mikemccandless.com> wrote: > Welcome Erick! > > Because nextHighestPowerOfTwo methods are public, I think we cannot > change what they return, nor remove them. At most we could deprecate > them now (and remove in 3.0), though I think it's fine to simply keep > them around even though nothing inside Lucene uses them today: since > we are heavy users of BitSet/Vector/Array/etc., it seems possible > we'll need them at some point. > > EG we are looking to make a better data structure to share > nearly-identical deleted doc bitsets between near realtime readers, > which could conceivably use advanced BitUtil methods. > > Keep hacking ;) > > Mike > > On Wed, Apr 22, 2009 at 9:33 PM, Erick Erickson <erickerick...@gmail.com> > wrote: > > Hi all: > > > > I've been participating in the user list for some time, and I'd like > > to start helping maintain/enhance the code. So I thought I'd start > > with something small, mostly to get the process down. Unit tests > > sure fit the bill it seems to me, less chance of introducing errors > > through ignorance but a fine way to extend *my* understanding > > of Lucene. > > > > I managed to check out the code and run the unit tests, which > > was amazingly easy. I even managed to get the project into > > IntelliJ and connect the codestyle.xml file. Kudos for whoever > > set up the checkout/build process, I was dreading spending > > days setting this up, fortunately I didn't have to. > > > > So I, with Chris's help, found the code coverage report and > > chose something pretty straightforward to test, BitUtil since it > > was nice and self-contained. As I said, I'm looking at understanding > > the process rather than adding much value the first time. > > > > Alas, even something as simple as BitUtil generates questions > > that I'm asking mostly to understand what approach the veterans > > prefer. I'll argue with y'all next year sometime <G>. > > > > So, according to the coverage report, there are two methods that > > are never executed by the unit tests (actually 4, 2 that operate on > > ints and 2 that operate on longs), isPowerOfTwo and > > nextHighestPowerOfTwo. nextHighestPowerOfTwo is especially > > clever, had to get out a paper and pencil to really understand it. > > > > Issues: > > 1> none of these methods is ever called. I commented them out > > and ran all the unit tests and all is well. Additionally, commenting > > out one of the other methods produces compile-time errors so I'm > > fairly sure I didn't do something completely stupid that just > *looked* > > like it was OK. I grepped recursively and they're nowhere in the > > *.java files. > > 1a> What's the consensus about unused code? Take it out (my > > preference) along with leaving a comment on where it can > > be found (since it *is* clever code)? Leave it in because > someone > > found some pretty neat algorithms that we may need sometime? > > 1b> I'm not entirely sure about the contrib area, but the contrib jars > > are all new so I assume "ant clean test" compiles them as well. > > > > 2> I don't agree with the behavior of nextHighestPowerOfTwo. Should > > I make changes if we decide to keep it? > > 2a> Why should it return the parameter passed in when it happens to be > > a perfect power of two? e.g. this passes: > > assertEquals(BitUtil.nextHighestPowerOfTwo(128L), 128); > > I'd expect this to actually return 256, given the name. > > 2b> Why should it ever return 0? There's no power of two that is > > zero. e.g. this passes: > > assertEquals(BitUtil.nextHighestPowerOfTwo(-1), 0); > > as does this: assertEquals(BitUtil.nextHighestPowerOfTwo(0), 0). > > *Assuming* that someone wants to use this sometime to, say, size > > an array they'd have to test against a return of 0. > > > > > > I'm fully aware that these are trivial issues in the grand scheme of > things, > > and I *really* don't want to waste much time hashing them over. I'll > provide > > a patch either way and go on to something slightly more complicated for > > my next trick. > > > > Best > > Erick > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: java-dev-h...@lucene.apache.org > >
