lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Erick Erickson <erickerick...@gmail.com>
Subject Re: Greetings and questions about patches
Date Thu, 23 Apr 2009 13:26:22 GMT
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 <
lucene@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 <erickerickson@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-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
>

Mime
View raw message