You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/01/08 10:25:58 UTC
[GitHub] [commons-crypto] arturobernalg opened a new pull request #129: Minor Improvements:
arturobernalg opened a new pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129
* collapsed Same Catch
* add final var
* Add benchmark dependency globally
* Add missing javadoc
* PMD:
** Use ConcurrentHashMap
** Avoid FileStream
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg closed pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
arturobernalg closed pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on a change in pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129#discussion_r553881334
##########
File path: src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
##########
@@ -74,7 +76,7 @@ public OsCryptoRandom(final Properties props) {
try {
close();
- this.stream = new FileInputStream(randomDevFile);
+ this.stream = Files.newInputStream(Paths.get(randomDevFile.getPath()));
Review comment:
I'm not 100% sure about this change.
##########
File path: src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java
##########
@@ -30,7 +32,7 @@
*/
public final class ReflectionUtils {
- private static final Map<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new WeakHashMap<>();
+ private static final ConcurrentMap<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new ConcurrentHashMap<>();
Review comment:
HI @garydgregory
In order to solve the PMD warning --> https://pmd.github.io/latest/pmd_rules_java_multithreading.html#useconcurrenthashmap
##########
File path: pom.xml
##########
@@ -731,5 +731,18 @@ The following provides more details on the included cryptographic software:
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
+ <!-- benchmark -->
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-core</artifactId>
+ <version>${jmh.version}</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-generator-annprocess</artifactId>
+ <version>${jmh.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
It wouldn't let me debug. I always getting error because it was not possible find all the package org.openjdk.jmh
##########
File path: src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
##########
@@ -74,7 +76,7 @@ public OsCryptoRandom(final Properties props) {
try {
close();
- this.stream = new FileInputStream(randomDevFile);
+ this.stream = Files.newInputStream(Paths.get(randomDevFile.getPath()));
Review comment:
OK @garydgregory
I close the PR and I will review the code
TY.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129#discussion_r553931692
##########
File path: src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java
##########
@@ -30,7 +32,7 @@
*/
public final class ReflectionUtils {
- private static final Map<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new WeakHashMap<>();
+ private static final ConcurrentMap<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new ConcurrentHashMap<>();
Review comment:
Why?
##########
File path: src/main/java/org/apache/commons/crypto/cipher/JceCipher.java
##########
@@ -35,6 +35,9 @@
* Implements the {@link CryptoCipher} using JCE provider.
*/
class JceCipher implements CryptoCipher {
+ /**
+ * The Cipher {@link Cipher} object.
Review comment:
Double word.
##########
File path: pom.xml
##########
@@ -731,5 +731,18 @@ The following provides more details on the included cryptographic software:
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
+ <!-- benchmark -->
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-core</artifactId>
+ <version>${jmh.version}</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-generator-annprocess</artifactId>
+ <version>${jmh.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
Why add this if there are no benchmarks in this PR?
##########
File path: src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java
##########
@@ -60,11 +60,11 @@
protected final byte[] data = new byte[dataLen];
protected byte[] encData;
private final Properties props = new Properties();
- protected byte[] key = new byte[16];
- protected byte[] iv = new byte[16];
- protected int count = 10000;
- protected static int defaultBufferSize = 8192;
- protected static int smallBufferSize = 1024;
+ protected final byte[] key = new byte[16];
+ protected final byte[] iv = new byte[16];
+ protected final int count = 10000;
+ protected static final int defaultBufferSize = 8192;
+ protected static final int smallBufferSize = 1024;
Review comment:
Don't break binary compatibility.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129#discussion_r553992160
##########
File path: src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java
##########
@@ -30,7 +32,7 @@
*/
public final class ReflectionUtils {
- private static final Map<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new WeakHashMap<>();
+ private static final ConcurrentMap<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new ConcurrentHashMap<>();
Review comment:
I don't think you've thought through the consequences of this change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on a change in pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129#discussion_r553940280
##########
File path: src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java
##########
@@ -30,7 +32,7 @@
*/
public final class ReflectionUtils {
- private static final Map<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new WeakHashMap<>();
+ private static final ConcurrentMap<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new ConcurrentHashMap<>();
Review comment:
HI @garydgregory
In order to solve the PMD warning --> https://pmd.github.io/latest/pmd_rules_java_multithreading.html#useconcurrenthashmap
##########
File path: pom.xml
##########
@@ -731,5 +731,18 @@ The following provides more details on the included cryptographic software:
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
+ <!-- benchmark -->
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-core</artifactId>
+ <version>${jmh.version}</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-generator-annprocess</artifactId>
+ <version>${jmh.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
It wouldn't let me debug. I always getting error because it was not possible find all the package org.openjdk.jmh
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg closed pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
arturobernalg closed pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129#discussion_r553931692
##########
File path: src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java
##########
@@ -30,7 +32,7 @@
*/
public final class ReflectionUtils {
- private static final Map<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new WeakHashMap<>();
+ private static final ConcurrentMap<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new ConcurrentHashMap<>();
Review comment:
Why?
##########
File path: src/main/java/org/apache/commons/crypto/cipher/JceCipher.java
##########
@@ -35,6 +35,9 @@
* Implements the {@link CryptoCipher} using JCE provider.
*/
class JceCipher implements CryptoCipher {
+ /**
+ * The Cipher {@link Cipher} object.
Review comment:
Double word.
##########
File path: pom.xml
##########
@@ -731,5 +731,18 @@ The following provides more details on the included cryptographic software:
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
+ <!-- benchmark -->
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-core</artifactId>
+ <version>${jmh.version}</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-generator-annprocess</artifactId>
+ <version>${jmh.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
Why add this if there are no benchmarks in this PR?
##########
File path: src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java
##########
@@ -60,11 +60,11 @@
protected final byte[] data = new byte[dataLen];
protected byte[] encData;
private final Properties props = new Properties();
- protected byte[] key = new byte[16];
- protected byte[] iv = new byte[16];
- protected int count = 10000;
- protected static int defaultBufferSize = 8192;
- protected static int smallBufferSize = 1024;
+ protected final byte[] key = new byte[16];
+ protected final byte[] iv = new byte[16];
+ protected final int count = 10000;
+ protected static final int defaultBufferSize = 8192;
+ protected static final int smallBufferSize = 1024;
Review comment:
Don't break binary compatibility.
##########
File path: src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java
##########
@@ -30,7 +32,7 @@
*/
public final class ReflectionUtils {
- private static final Map<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new WeakHashMap<>();
+ private static final ConcurrentMap<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new ConcurrentHashMap<>();
Review comment:
I don't think you've thought through the consequences of this change.
##########
File path: src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
##########
@@ -74,7 +76,7 @@ public OsCryptoRandom(final Properties props) {
try {
close();
- this.stream = new FileInputStream(randomDevFile);
+ this.stream = Files.newInputStream(Paths.get(randomDevFile.getPath()));
Review comment:
This is coded in a pretty obtuse manner, if the point is to use NIO, then start with a Path instead of a File.
In general, it feels to me like you need to step back and look at the big picture instead of applying whatever micro changes your IDE offers (or PMD, and so on).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on a change in pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129#discussion_r554095320
##########
File path: src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
##########
@@ -74,7 +76,7 @@ public OsCryptoRandom(final Properties props) {
try {
close();
- this.stream = new FileInputStream(randomDevFile);
+ this.stream = Files.newInputStream(Paths.get(randomDevFile.getPath()));
Review comment:
OK @garydgregory
I close the PR and I will review the code
TY.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on a change in pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129#discussion_r553881334
##########
File path: src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
##########
@@ -74,7 +76,7 @@ public OsCryptoRandom(final Properties props) {
try {
close();
- this.stream = new FileInputStream(randomDevFile);
+ this.stream = Files.newInputStream(Paths.get(randomDevFile.getPath()));
Review comment:
I'm not 100% sure about this change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #129: Minor Improvements:
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #129:
URL: https://github.com/apache/commons-crypto/pull/129#discussion_r554005510
##########
File path: src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
##########
@@ -74,7 +76,7 @@ public OsCryptoRandom(final Properties props) {
try {
close();
- this.stream = new FileInputStream(randomDevFile);
+ this.stream = Files.newInputStream(Paths.get(randomDevFile.getPath()));
Review comment:
This is coded in a pretty obtuse manner, if the point is to use NIO, then start with a Path instead of a File.
In general, it feels to me like you need to step back and look at the big picture instead of applying whatever micro changes your IDE offers (or PMD, and so on).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org