-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-44055: [Java] Finalize ErrorProne Warnings to be considered as Errors #44056
base: main
Are you sure you want to change the base?
Changes from all commits
0472277
369ad2d
2f91c9b
0e79ee9
de95e9c
a99a701
f8bc623
ab93ca8
7c1d829
003e1ab
ceaf567
5b85701
1354ad0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,12 +45,14 @@ static ErrorValue fromProtocol(Flight.SetSessionOptionsResult.ErrorValue s) { | |
return values()[s.getNumber()]; | ||
} | ||
|
||
@SuppressWarnings("EnumOrdinal") | ||
Flight.SetSessionOptionsResult.ErrorValue toProtocol() { | ||
return Flight.SetSessionOptionsResult.ErrorValue.values()[ordinal()]; | ||
} | ||
} | ||
|
||
/** Per-option extensible error response container. */ | ||
@SuppressWarnings("JavaLangClash") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What was the problem here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's clashing with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for renaming it, if it's not an external API change in some way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is public, it will be a breaking change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, the issue is as @danepitkin mentioned. Shall we leave the warning suppressed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let's leave the suppression in place. |
||
public static class Error { | ||
public ErrorValue value; | ||
|
||
|
@@ -74,7 +76,7 @@ public boolean equals(Object o) { | |
if (this == o) { | ||
return true; | ||
} | ||
if (o == null || getClass() != o.getClass()) { | ||
if (!(o instanceof Error)) { | ||
return false; | ||
} | ||
Error that = (Error) o; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,8 +70,8 @@ public class AddWritableBuffer { | |
tmpBufChainOut = tmpBufChainOut2; | ||
|
||
} catch (Exception ex) { | ||
new RuntimeException("Failed to initialize AddWritableBuffer, falling back to slow path", ex) | ||
.printStackTrace(); | ||
throw new RuntimeException( | ||
"Failed to initialize AddWritableBuffer, falling back to slow path", ex); | ||
Comment on lines
+73
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this intentionally did not throw, as evidenced by the "falling back" message |
||
} | ||
|
||
bufConstruct = tmpConstruct; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,8 +47,8 @@ public class GetReadableBuffer { | |
tmpField = f; | ||
tmpClazz = clazz; | ||
} catch (Exception e) { | ||
new RuntimeException("Failed to initialize GetReadableBuffer, falling back to slow path", e) | ||
.printStackTrace(); | ||
throw new RuntimeException( | ||
"Failed to initialize GetReadableBuffer, falling back to slow path", e); | ||
Comment on lines
+50
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
} | ||
READABLE_BUFFER = tmpField; | ||
BUFFER_INPUT_STREAM = tmpClazz; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the right way to do this would be to add a constructor and store the Protobuf enum value in the enum instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that is the ideal way. Let me track down and change the similar other instances.