uschindler commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780226954
##########
File path:
lucene/analysis/common/src/java/org/apache/lucene/analysis/core/DecimalDigitFilter.java
##########
@@ -35,6 +35,7 @@ public DecimalDigitFilter(TokenStream input) {
}
@Override
+ @SuppressWarnings("CharacterGetNumericValue") // we use
Character.isDigit(ch), so it is safe
Review comment:
You verified it is fine, so I will disable. Why are you always reacting
so angry?
##########
File path:
lucene/analysis/common/src/java/org/apache/lucene/analysis/core/DecimalDigitFilter.java
##########
@@ -35,6 +35,7 @@ public DecimalDigitFilter(TokenStream input) {
}
@Override
+ @SuppressWarnings("CharacterGetNumericValue") // we use
Character.isDigit(ch), so it is safe
Review comment:
If you look at the other changes to me this was shocking. We had tests
that converted longs to floats and compared them with an epsilon. To me this is
a bug.
The tests were passing because numbers in tests are small.
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -99,6 +99,7 @@ private static int randomInt(int bound) {
return bound == 0 ? 0 : random().nextInt(bound);
}
+ @SuppressWarnings("BareDotMetacharacter")
Review comment:
I think we can keep it in. I would generally put String.replaceAll on
the forbidden APIs list as this causes so many also security bugs. In Solr I
have seen this exact case where somebody wanted to replace dots in class names
by slashes.
Most people don't know that replace all uses a regex, which needs to be
compiled, too. So in production code I would prefer to always compile patterns
up front.
So as this is only one place we should keep it in. Maybe it catches other
shit.
Especially this piece of code on the first view looks crazy. Maybe rewrite
to be explicit using a pattern.
Maybe I should do this and remove the suppression.
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -99,6 +99,7 @@ private static int randomInt(int bound) {
return bound == 0 ? 0 : random().nextInt(bound);
}
+ @SuppressWarnings("BareDotMetacharacter")
Review comment:
I replaced this locally by `".".repeat(replacementPart.length())`, which
is much more readable than the previous `replaceAll(".", ".")`. The dot regex
alone looks too crazy.
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -99,6 +99,7 @@ private static int randomInt(int bound) {
return bound == 0 ? 0 : random().nextInt(bound);
}
+ @SuppressWarnings("BareDotMetacharacter")
Review comment:
I replaced this locally by `".".repeat(replacementPart.length())`, which
is much more readable than the previous `replaceAll(".", ".")` (new API in Java
11, very fast for chars). The dot regex alone looks too crazy.
This is unrelated to forbiddenapis.
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -99,6 +99,7 @@ private static int randomInt(int bound) {
return bound == 0 ? 0 : random().nextInt(bound);
}
+ @SuppressWarnings("BareDotMetacharacter")
Review comment:
> I'd really prefer we implement such checks outside of the
nightly-only-slow-errorprone
Thats a different discussion unrleted to this issue.
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -99,6 +99,7 @@ private static int randomInt(int bound) {
return bound == 0 ? 0 : random().nextInt(bound);
}
+ @SuppressWarnings("BareDotMetacharacter")
Review comment:
committed.
--
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
