You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/01/07 11:36:05 UTC
[GitHub] [lucene] uschindler opened a new pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
uschindler opened a new pull request #590:
URL: https://github.com/apache/lucene/pull/590
See https://issues.apache.org/jira/browse/LUCENE-10364
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780321286
##########
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:
uhhh, it is definitely related to this issue. this issue is the thing enabling the new check! so I'm trying to suggest a better implementation vs turning on an annoyance.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780223468
##########
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:
Please, disable this check from error prone completely. it has 100% false positives and is useless. There is nothing wrong with this using this method.
##########
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:
I just asked to ban this check because it adds useless noise. I'm not a fan of these kinds of error-prone checks that have a high false-positive rate.
IMO, if we consider methods a trap, I'd rather use forbidden-apis. And if we want to lint the code, we should crank up javac and ecj warnings (there are still stuff there we can enable).
It is unclear to me where error-prone fits in... but let's be selective about which of its crazy heuristical checks gets applied? If there are ones that truly find bugs, that's different than if we are just making our build system annoying and fighting the developers.
##########
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:
I'm just arguing that this particular check isn't useful.
It is "dumb" in that it seems to just fail if you use the method at all:
https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/CharacterGetNumericValue.java
It doesn't do any "real analysis" to see if e.g. code is guarded by `Character.isDigit` check or similar.
Again, if we want to ban methods outright, i'd rather use forbidden APIs. It is a million times faster, and it runs non-nightly and is more friendly to the developers. Separately, I don't think this particular method needs to be banned though.
##########
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:
Should we disable this check? It seems sketchy to me: http://errorprone.info/bugpattern/BareDotMetacharacter
##########
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:
could we disable the check and just add String.replaceAll to forbidden instead?
I'd really prefer we implement such checks outside of the nightly-only-slow-errorprone, if possible. It means less tension between builds/failures and developers
##########
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:
uhhh, it is definitely related to this issue. this issue is the thing enabling the new check! so I'm trying to suggest a better implementation vs turning on an annoyance.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780231918
##########
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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780309213
##########
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780306400
##########
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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
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?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780260497
##########
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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #590:
URL: https://github.com/apache/lucene/pull/590#issuecomment-1007484292
Hi I also crosschecked with the original list after sorting them and verified: All warnings previously disabled are still disabled or obsolete because fixed already, I only added new ones.
So I think this is ready.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780281728
##########
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:
could we disable the check and just add String.replaceAll to forbidden instead?
I'd really prefer we implement such checks outside of the nightly-only-slow-errorprone, if possible. It means less tension between builds/failures and developers
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler merged pull request #590:
URL: https://github.com/apache/lucene/pull/590
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780247990
##########
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:
Should we disable this check? It seems sketchy to me: http://errorprone.info/bugpattern/BareDotMetacharacter
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #590:
URL: https://github.com/apache/lucene/pull/590#issuecomment-1007467493
I now ordered the warning list alphabetically to make them easier to manage
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780223468
##########
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:
Please, disable this check from error prone completely. it has 100% false positives and is useless. There is nothing wrong with this using this method.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780229999
##########
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:
I just asked to ban this check because it adds useless noise. I'm not a fan of these kinds of error-prone checks that have a high false-positive rate.
IMO, if we consider methods a trap, I'd rather use forbidden-apis. And if we want to lint the code, we should crank up javac and ecj warnings (there are still stuff there we can enable).
It is unclear to me where error-prone fits in... but let's be selective about which of its crazy heuristical checks gets applied? If there are ones that truly find bugs, that's different than if we are just making our build system annoying and fighting the developers.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780306400
##########
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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780308441
##########
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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler merged pull request #590:
URL: https://github.com/apache/lucene/pull/590
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780235407
##########
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:
I'm just arguing that this particular check isn't useful.
It is "dumb" in that it seems to just fail if you use the method at all:
https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/CharacterGetNumericValue.java
It doesn't do any "real analysis" to see if e.g. code is guarded by `Character.isDigit` check or similar.
Again, if we want to ban methods outright, i'd rather use forbidden APIs. It is a million times faster, and it runs non-nightly and is more friendly to the developers. Separately, I don't think this particular method needs to be banned though.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #590: LUCENE-10364: Prepare and update errorprone plugin for Java 17
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #590:
URL: https://github.com/apache/lucene/pull/590#issuecomment-1007467493
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org