lidavidm commented on code in PR #37723:
URL: https://github.com/apache/arrow/pull/37723#discussion_r1330578945
##########
java/pom.xml:
##########
@@ -44,8 +44,11 @@
<forkCount>2</forkCount>
<checkstyle.failOnViolation>true</checkstyle.failOnViolation>
<errorprone.javac.version>9+181-r4173-1</errorprone.javac.version>
- <error_prone_core.version>2.16</error_prone_core.version>
- <maven-compiler-plugin.version>3.10.1</maven-compiler-plugin.version>
+ <pinjdk8.errorprone.core.version>2.10.0</pinjdk8.errorprone.core.version>
Review Comment:
More reason to drop JDK8 ASAP.
##########
java/pom.xml:
##########
@@ -767,45 +774,99 @@
</profile>
<profile>
- <id>error-prone-jdk8</id>
- <!--
- Do not activate Error Prone while running with Eclipse/M2E as it
causes incompatibilities
- with other annotation processors.
- See https://github.com/jbosstools/m2e-apt/issues/62 for details
- -->
+ <id>error-prone-checkerframework-jdk8</id>
Review Comment:
IMO, we don't bother enabling this for JDK8. In fact, I'd be OK with
dropping error-prone for JDK8 as well, and only enabling these checks only on
the latest LTS.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -42,7 +43,7 @@ abstract class BaseAllocator extends Accountant implements
BufferAllocator {
public static final String DEBUG_ALLOCATOR = "arrow.memory.debug.allocator";
public static final int DEBUG_LOG_LENGTH = 6;
- public static final boolean DEBUG;
+ public static final @Nullable boolean DEBUG;
Review Comment:
This also seems wrong.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java:
##########
@@ -1033,7 +1034,7 @@ public void getBytes(long index, OutputStream out, int
length) throws IOExceptio
// copy length bytes of data from this ArrowBuf starting at
// address addr(index) into the tmp byte array starting at index 0
byte[] tmp = new byte[length];
- MemoryUtil.UNSAFE.copyMemory(null, addr(index), tmp,
MemoryUtil.BYTE_ARRAY_BASE_OFFSET, length);
Review Comment:
nit: is it possible to make the `UNSAFE` constant `private` to avoid other
problems like this? (We can do that in a separate ticket.)
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -183,6 +186,7 @@ public ArrowBuf getEmpty() {
* we have a new ledger
* associated with this allocator.
*/
+ @SuppressWarnings({"nullness:locking.nullable", "nullness:argument"})
Review Comment:
What are the warnings here?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -154,7 +157,7 @@ private static String createErrorMsg(final BufferAllocator
allocator, final long
}
}
- public static boolean isDebug() {
+ public static @Nullable boolean isDebug() {
Review Comment:
This seems wrong: this is a primitive and should never be null.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java:
##########
@@ -81,7 +82,8 @@ public ArrowBufPointer(ArrowBuf buf, long offset, long
length) {
* @param length the length off set of the memory region pointed to.
* @param hasher the hasher used to calculate the hash code.
*/
- public ArrowBufPointer(ArrowBuf buf, long offset, long length,
ArrowBufHasher hasher) {
+ @SuppressWarnings("initialization.fields.uninitialized")
Review Comment:
Can we initialize fields instead of suppressing the warnings?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -64,15 +65,16 @@ abstract class BaseAllocator extends Accountant implements
BufferAllocator {
// Package exposed for sharing between AllocatorManger and BaseAllocator
objects
private final String name;
private final RootAllocator root;
- private final Object DEBUG_LOCK = DEBUG ? new Object() : null;
+
+ private final @Nullable Object DEBUG_LOCK = DEBUG ? new Object() : null;
Review Comment:
IMO, there is no harm in always initializing this.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java:
##########
@@ -33,7 +35,7 @@
public class MemoryUtil {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(MemoryUtil.class);
- private static final Constructor<?> DIRECT_BUFFER_CONSTRUCTOR;
+ private static final @Nullable Constructor<?> DIRECT_BUFFER_CONSTRUCTOR;
Review Comment:
Does Checker Framework have anything for static initializers?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
