-
Notifications
You must be signed in to change notification settings - Fork 827
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
Use relatively safe max size of java array #516
Conversation
Is there a reason why the constants in Util aren't marked final? If the code relies on their mutability that is a significant threading issue. I know you are just following the model of what has been done before in that class, but I'm wondering if this should be fixed. |
dd39a84
to
36a53bb
Compare
@isaki-x valid question. In the mean time I will fix this in the current PR. Thanks. |
@@ -26,6 +26,9 @@ | |||
/** A few utility methods, mostly for private use. | |||
* @author Nathan Sweet <misc@n4te.com> */ | |||
public class Util { | |||
//See: https://stackoverflow.com/questions/3038392/do-java-arrays-have-a-maximum-size | |||
static public final int maxSafeArraySize = Integer.MAX_VALUE - 8; |
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.
As a constant it should probably follow naming conventions for constants?
Alternatively, this could be defined somewhere in the io package and be package private, with the advantage that the public api is not extended (but keeping it here is fine for me as well).
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.
The issue here is that while you are certainly correct, the file being modified does not in fact utilize the standard (so it becomes a style issue: should the file be updated to match the standard or should it be left alone and made at least internally consistent?).
The line after the line the author added is:
static public boolean isAndroid = "Dalvik".equals(System.getProperty("java.vm.name"));
This should probably be changed to:
public static final boolean IS_ANDROID = "Dalvik".equals(System.getProperty("java.vm.name"));
... or something along those lines. Unless of course you intended for this to be modifiable... then you should probably have set/get and locks/synchronized access or make it an AtomicBoolean (or an AtomicBoolean abstracted by get/set so it can change later).
I do like the idea of minimal visibility and moving this to the IO package is probably the right call regardless of the naming standard and thread safety considerations I have raised.
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.
Oh yeah, you're right, I should have made this final - I don't think that I intended this to be modifiable (I really hope that nobody wants to do this) ;-)
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.
36a53bb
to
9ddf7cb
Compare
9ddf7cb
to
389d33a
Compare
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.
Thanks for the PR!
See: