FSchumacher commented on code in PR #5873:
URL: https://github.com/apache/jmeter/pull/5873#discussion_r1181616269
##########
build-logic/verification/src/main/kotlin/build-logic.errorprone.gradle.kts:
##########
@@ -38,11 +38,28 @@ if (buildParameters.enableErrorprone) {
// Ignore warnings in test code
options.errorprone.isEnabled.set(false)
} else {
+ // Errorprone requires Java 11+
+ options.errorprone.isEnabled.set(
+ javaCompiler.map {
it.metadata.languageVersion.canCompileOrRun(11) }
+ )
options.compilerArgs.addAll(listOf("-Xmaxerrs", "10000",
"-Xmaxwarns", "10000"))
options.errorprone {
disableWarningsInGeneratedCode.set(true)
enable(
- "PackageLocation"
+ "MissingDefault",
+ "PackageLocation",
+ "RedundantOverride",
+ "StronglyTypeTime",
+ "UnescapedEntity",
+ "UnnecessaryAnonymousClass",
+ "UnnecessaryDefaultInEnumSwitch",
+ )
+ warn(
+ // "FieldCanBeFinal",
Review Comment:
Is this the same as on line 60?
##########
src/components/src/main/java/org/apache/jmeter/assertions/CompareAssertion.java:
##########
@@ -126,8 +126,8 @@ private void compareContent(CompareAssertionResult result) {
}
}
- private void markContentFailure(CompareAssertionResult result, String
prevContent, SampleResult prevResult,
- SampleResult currentResult, String currentContent) {
+ private static void markContentFailure(CompareAssertionResult result,
String prevContent, SampleResult prevResult,
Review Comment:
In my current IDE setup, the IDE suggests this kind of change quite often
and I wonder why. Is it better performing, more stylish, something else?
##########
src/functions/src/main/java/org/apache/jmeter/functions/ChangeCase.java:
##########
@@ -90,8 +90,6 @@ protected String changeCase(String originalString, String
mode) {
case CAPITALIZE:
targetString = StringUtils.capitalize(originalString);
break;
- default:
Review Comment:
This seems to be OK
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPAbstractImpl.java:
##########
@@ -246,7 +246,6 @@ protected InetAddress getIpSourceAddress() throws
UnknownHostException, SocketEx
ipClass = Inet6Address.class;
break;
case HOSTNAME:
- default:
Review Comment:
Is this correct?
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/parser/LagartoBasedHtmlParser.java:
##########
@@ -181,8 +181,6 @@ public void tag(Tag tag) {
break;
case END:
break;
- default:
Review Comment:
Seems wrong to remove
##########
src/core/src/main/java/org/apache/jmeter/gui/action/AbstractAction.java:
##########
@@ -102,7 +102,6 @@ protected boolean popupCheckExistingFileListener(HashTree
tree) {
return false;
}
case ASK:
- default:
Review Comment:
While it might not be nice to have a fall-through from `ASK` to `default`.
Removing default seems wrong (haven't looked deeply into this).
##########
src/jorphan/src/main/java/org/apache/jorphan/gui/MenuScroller.java:
##########
@@ -514,19 +514,6 @@ public void dispose() {
}
}
- /**
- * Ensures that the <code>dispose</code> method of this MenuScroller is
- * called when there are no more references to it.
- *
- * @see MenuScroller#dispose()
- */
- @Override
- @SuppressWarnings("deprecation")
- public void finalize() throws Throwable {
- dispose();
Review Comment:
Can we remove this already? Do we have to take care of dspose() somewhere
else?
##########
src/functions/src/main/java/org/apache/jmeter/functions/LogFunction.java:
##########
@@ -175,8 +175,6 @@ static synchronized void logDetails(Logger logger, String
stringToLog, String pr
case TRACE:
logger.trace("{} {} {}", threadName, separator,
stringToLog, throwable);
break;
- default:
Review Comment:
Seems wrong to be removed.
##########
src/core/src/main/java/org/apache/jmeter/report/processor/ExternalSampleSorter.java:
##########
@@ -91,9 +91,9 @@ public class ExternalSampleSorter extends
AbstractSampleConsumer {
private final BlockingQueue<Runnable> workQueue = new
LinkedBlockingQueue<>();
- private ThreadPoolExecutor pool;
+ private final ThreadPoolExecutor pool;
- private volatile int nbProcessors;
+ private final int nbProcessors;
Review Comment:
Again a strange conversion from `volatile` to `final`.
##########
src/core/src/main/java/org/apache/jmeter/save/CSVSaveService.java:
##########
@@ -1075,8 +1075,6 @@ public static String[] csvReadFile(BufferedReader infile,
char delim)
+ baos.toString() + "]");
}
break;
- default:
Review Comment:
Why is it removed?
##########
src/components/src/main/java/org/apache/jmeter/visualizers/SearchTextExtension.java:
##########
@@ -244,11 +244,11 @@ public interface ISearchTextExtensionProvider {
*/
public static class JEditorPaneSearchProvider implements
ISearchTextExtensionProvider {
- private static volatile int LAST_POSITION_DEFAULT = 0;
+ private static final int LAST_POSITION_DEFAULT = 0;
Review Comment:
This seems to be a strange diff. `volatile` seems to indicate, that it is
rather not a good candidate to be `final`.
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/ProxyControl.java:
##########
@@ -1532,8 +1532,6 @@ private void initKeyStore() throws IOException,
GeneralSecurityException {
break;
case NONE:
throw new IOException("Cannot find keytool application and no
keystore was provided");
- default:
Review Comment:
Again, is this correct?
--
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: dev-unsubscr...@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
