[GitHub] [accumulo] ctubbsii commented on a change in pull request #2558: Specify when return values are unused

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r823044360



##########
File path: core/src/main/java/org/apache/accumulo/core/client/summary/CounterSummary.java
##########
@@ -54,7 +54,8 @@ public CounterSummary(Summary summary, boolean checkType) {
     if (checkType) {
       String className = summary.getSummarizerConfiguration().getClassName();
       try {
-        getClass().getClassLoader().loadClass(className).asSubclass(CountingSummarizer.class);
+        var unusedRetVal =
+            getClass().getClassLoader().loadClass(className).asSubclass(CountingSummarizer.class);

Review comment:
       This might make errorprone happy, but it adds compiler warnings about unused variables, at least in Eclipse (maybe also with javac?). One option is to actually use it. Could add a trace log message that uses it after it is assigned.

##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -468,7 +468,7 @@ public void testDeniedAudits() throws AccumuloSecurityException, AccumuloExcepti
       auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME);
     } catch (AccumuloSecurityException ex) {}
     try (Scanner scanner = auditAccumuloClient.createScanner(OLD_TEST_TABLE_NAME, auths)) {
-      scanner.iterator().next().getKey();
+      var unusedRetVal = scanner.iterator().next().getKey();
     } catch (RuntimeException ex) {}

Review comment:
       It looks like this is a pattern where we are expecting the error. This should be refactored to use `assertThrows` instead of trying to ignore the return value.

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -91,7 +91,7 @@ public boolean equals(Object o) {
   }
 
   public final void precomputeHashCode() {
-    hashCode();
+    var unusedRetVal = hashCode();

Review comment:
       Instead of doing this, could just do this computation in the constructor and delete this method. The only place where this is used is immediately after construction in one case, and the only other time this object is constructed is in ServerClient where it is not going to hurt to precompute.

##########
File path: test/src/main/java/org/apache/accumulo/test/SampleIT.java
##########
@@ -490,7 +490,7 @@ private void assertSampleNotPresent(SamplerConfiguration sc, ScannerBase... scan
 
       scanner.clearSamplerConfiguration();
       for (Entry<Key,Value> entry : scanner) {
-        entry.getKey();
+        var unusedRetVal = entry.getKey();

Review comment:
       The call to `.getKey` was only ever added to avoid a previous warning about the loop variable being unused. It would probably be sufficient to have an assert that the loop variable is non-null.

##########
File path: test/src/main/java/org/apache/accumulo/test/CloseScannerIT.java
##########
@@ -58,7 +58,7 @@ public void testManyScans() throws Exception {
 
           for (int j = 0; j < i % 7 + 1; j++) {
             // only read a little data and quit, this should leave a session open on the tserver
-            Iterators.get(scanner.iterator(), 10);
+            var unusedRetVal = Iterators.get(scanner.iterator(), 10);

Review comment:
       An assertNotNull() or similar around this would probably suffice.

##########
File path: test/src/main/java/org/apache/accumulo/test/DetectDeadTabletServersIT.java
##########
@@ -56,7 +56,8 @@ protected void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSit
   public void test() throws Exception {
     try (AccumuloClient c = Accumulo.newClient().from(getClientProperties()).build()) {
       log.info("verifying that everything is up");
-      Iterators.size(c.createScanner(MetadataTable.NAME, Authorizations.EMPTY).iterator());
+      var unusedRetVal =
+          Iterators.size(c.createScanner(MetadataTable.NAME, Authorizations.EMPTY).iterator());

Review comment:
       This could verify the size is `> 0`.




-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org