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
