You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by "Uwe Schindler (Issue Comment Edited) (JIRA)" <ji...@apache.org> on 2012/03/17 15:11:37 UTC
[jira] [Issue Comment Edited] (LUCENE-3738) Be consistent about
negative vInt/vLong
[ https://issues.apache.org/jira/browse/LUCENE-3738?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13231978#comment-13231978 ]
Uwe Schindler edited comment on LUCENE-3738 at 3/17/12 2:10 PM:
----------------------------------------------------------------
{quote}
bq. The check is only ommitted in the unrolled loop, the for-loop still contains the check.
I'm confused... I don't see how/where BufferedIndexInput.readVLong is
checking for negative result now...?
{quote}
I mean that the actual "if (b & mask \!= 0)" is also in the original while loop. The original while loop then simply proceeds with reading bytes util the highest bit is null. The unrolled loop behaves different (and thats a real bug), because it will silently not read those remaining bytes, so the file pointer is on a different byte after the call. This also affects readVInt!!!
In my opinion, we should unroll *all* readVInt/readVLong loops so all behave 100% identical! And in the case of the last byte read (where the current assert is), throw exception. If we don't unroll all readVInts we have to somehow also make the loop exit after too many bytes are read, which would be an costly extra check in the loop - thats the reason why I want to unroll all loops to fail after 5 or 9 bytes.
was (Author: thetaphi):
{quote}
bq. The check is only ommitted in the unrolled loop, the for-loop still contains the check.
I'm confused... I don't see how/where BufferedIndexInput.readVLong is
checking for negative result now...?
{quote}
I mean that the actual "if (b & mask != 0)" is also in the original while loop. The original while loop then simply proceeds with reading bytes util the highest bit is null. The unrolled loop behaves different (and thats a real bug), because it will silently not read those remaining bytes, so the file pointer is on a different byte after the call. This also affects readVInt!!!
In my opinion, we should unroll *all* readVInt/readVLong loops so all behave 100% identical! And in the case of the last byte read (where the current assert is), throw exception. If we don't unroll all readVInts we have to somehow also make the loop exit after too many bytes are read, which would be an costly extra check in the loop - thats the reason why I want to unroll all loops to fail after 5 or 9 bytes.
> Be consistent about negative vInt/vLong
> ---------------------------------------
>
> Key: LUCENE-3738
> URL: https://issues.apache.org/jira/browse/LUCENE-3738
> Project: Lucene - Java
> Issue Type: Bug
> Reporter: Michael McCandless
> Assignee: Uwe Schindler
> Fix For: 3.6, 4.0
>
> Attachments: LUCENE-3738.patch, LUCENE-3738.patch, LUCENE-3738.patch
>
>
> Today, write/readVInt "allows" a negative int, in that it will encode and decode correctly, just horribly inefficiently (5 bytes).
> However, read/writeVLong fails (trips an assert).
> I'd prefer that both vInt/vLong trip an assert if you ever try to write a negative number... it's badly trappy today. But, unfortunately, we sometimes rely on this... had we had this assert in 'since the beginning' we could have avoided that.
> So, if we can't add that assert in today, I think we should at least fix readVLong to handle negative longs... but then you quietly spend 9 bytes (even more trappy!).
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org