[GitHub] [thrift] ctubbsii commented on a diff in pull request #2575: THRIFT-5563: fix deprecation and enable xlint for java library

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

ctubbsii commented on code in PR #2575:
URL: https://github.com/apache/thrift/pull/2575#discussion_r851867229


##########
lib/java/gradle/sourceConfiguration.gradle:
##########
@@ -60,7 +60,16 @@ tasks.withType(JavaCompile) {
     if (JavaVersion.current() > JavaVersion.VERSION_1_8) {
         options.compilerArgs.addAll(['--release', '8'])
     }
-    // options.compilerArgs.addAll('-Xlint:unchecked')
+    options.compilerArgs.addAll([
+            '-Werror',
+            '-Xlint:deprecation',
+            '-Xlint:cast',
+            '-Xlint:empty',
+            '-Xlint:fallthrough',
+            '-Xlint:finally',
+            '-Xlint:overrides',
+            // we can't enable -Xlint:unchecked just yet

Review Comment:
   There might be a shorthand for this... something like: `-Xlint:all,-unchecked`
   However, I haven't experimented with this enough to know for sure.



##########
lib/java/src/org/apache/thrift/partial/ThriftMetadata.java:
##########
@@ -463,14 +463,12 @@ protected void toPrettyString(StringBuilder sb, int level) {
     }
 
     public <T extends TBase> T createNewStruct() {
-      T instance = null;
+      T instance;
 
       try {
         Class<T> structClass = getStructClass(this.data);
-        instance = (T) structClass.newInstance();
-      } catch (InstantiationException e) {
-        throw new RuntimeException(e);
-      } catch (IllegalAccessException e) {
+        instance = structClass.newInstance();

Review Comment:
   Should this use `.getDeclaredConstructor().newInstance()`? Or is that too new for Thrift's minimum Java version?



##########
lib/java/src/org/apache/thrift/partial/ThriftMetadata.java:
##########
@@ -463,14 +463,12 @@ protected void toPrettyString(StringBuilder sb, int level) {
     }
 
     public <T extends TBase> T createNewStruct() {
-      T instance = null;
+      T instance;
 
       try {
         Class<T> structClass = getStructClass(this.data);
-        instance = (T) structClass.newInstance();
-      } catch (InstantiationException e) {
-        throw new RuntimeException(e);
-      } catch (IllegalAccessException e) {
+        instance = structClass.newInstance();
+      } catch (InstantiationException | IllegalAccessException e) {

Review Comment:
   These could just catch all `ReflectiveOperationException`



-- 
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@thrift.apache.org

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