Yah that's a fun one... keep prompting! I just committed some comments around the +1 so the future "we" "remember" why it's there...
Mike On Sun, Dec 6, 2009 at 7:27 PM, Erick Erickson <erickerick...@gmail.com> wrote: > Should have mentioned in my first message that all I > was really after was prompting folks who actually know > something about the code in question to avoid > the mistake I've made, oh, several thousand times... > > "There's no reason for that to be there, I'll just take > it out" <G>.... > > Erick > > On Sun, Dec 6, 2009 at 6:45 PM, Michael McCandless > <luc...@mikemccandless.com> wrote: >> >> On Sun, Dec 6, 2009 at 5:51 PM, Uwe Schindler <u...@thetaphi.de> wrote: >> >> On Sun, Dec 06, 2009 at 05:31:53PM -0500, Erick Erickson wrote: >> >> > This may be a silly question, and I admit that I haven't looked a the >> >> code, >> >> > but was there a good reason to +1 it in the first place or was that >> >> > just >> >> > paranoia to prevent off-by-one errors? >> >> >> >> IIRC, this implementation of the priority queue algo leaves open slot 0 >> >> to >> >> simplify internal calculations. It was that way when I ported 1.4.3, >> >> and >> >> I >> >> doubt that's changed. >> > >> > Thats still the same. Because calculations in heaps are simplier when >> > 1-based. Because of that heap[0] is unused. >> >> Thanks for raising this Erick... it's a good question. Technically, >> removing the +1 would be a bug if anyone ever inserted 2B items into >> the PQ, but I think this is exceptionally unlikely to occur in >> practice. >> >> >> > If there *was* a valid reason, might it make sense to >> >> > +1 min(nDocs, maxDoc())? >> >> >> >> I think the patch is fine. It's really only needed to provide a more >> >> accurate >> >> error message in the event somebody specifies that they want >> >> Integer.MAX_VALUE >> >> elements, not realizing that they will be allocated up front rather >> >> than >> >> lazily -- they'll get an OOME rather than a NegativeArraySizeException. >> > >> > The new patch is more intelligent, it will not allocate such a big queue >> > as >> > far as I have seen. It takes the numDocs() of index reader/searcher into >> > account. >> >> Hmm actually it takes maxDoc() into account, but it should in fact use >> numDocs(). I'll fix. >> >> Mike >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org >> For additional commands, e-mail: java-dev-h...@lucene.apache.org >> > > --------------------------------------------------------------------- To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org For additional commands, e-mail: java-dev-h...@lucene.apache.org
