mikemccand commented on code in PR #858:
URL: https://github.com/apache/lucene/pull/858#discussion_r862846952
##########
lucene/core/src/java/org/apache/lucene/util/compress/LowercaseAsciiCompression.java:
##########
@@ -111,14 +111,13 @@ public static boolean compress(byte[] in, int len, byte[]
tmp, DataOutput out)
numExceptions2++;
}
}
+
+ // TODO: shouldn't this really be an assert instead? but then this real
"if" triggered
+ // LUCENE-10551 so maybe it should remain a real "if":
Review Comment:
+1, that logic makes sense to me -- it's an `assert` at heart (its purpose
is to find bugs in our code, not let the caller know that something went wrong
with their inputs), but for (clearly well-founded!) paranoia and low-cost we
upgraded it to a real `if`.
But maybe to make it clear that it is a "paranoid upgraded `assert` flavored
`if`" we should throw `AssertionError` instead? Because the user who opened
the issue thought this was a case of not being able to compress the string and
we should return `false`, but rather it is a case of bug-in-something. If they
saw the `AssertionError` maybe they would realize it's a bug-in-something (a
BIS)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
