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/06/15 16:34:42 UTC
[GitHub] [commons-imaging] arturobernalg opened a new pull request #155: Remove redundant local variable.
arturobernalg opened a new pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155
--
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-imaging] coveralls edited a comment on pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#issuecomment-861667397
[](https://coveralls.io/builds/41087684)
Coverage decreased (-0.008%) to 76.657% when pulling **e1fc5410c43bdbf9ae176570d8f7514e853da88b on arturobernalg:feature/inline** into **c030b47ff32652b273cc8e3e856ab7e3ff62682d on apache:master**.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] arturobernalg commented on a change in pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r665700494
##########
File path: src/main/java/org/apache/commons/imaging/formats/png/PngImageParser.java
##########
@@ -254,9 +254,8 @@ public void readSignature(final InputStream is) throws ImageReadException,
}
final PngChunkIccp pngChunkiCCP = (PngChunkIccp) chunks.get(0);
- final byte[] bytes = pngChunkiCCP.getUncompressedProfile(); // TODO should this be a clone?
- return (bytes);
+ return (pngChunkiCCP.getUncompressedProfile());
Review comment:
Reverted
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #155: [IMAGING-315] Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#issuecomment-950119855
Merged, thanks!
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] coveralls edited a comment on pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#issuecomment-861667397
[](https://coveralls.io/builds/41198985)
Coverage decreased (-0.002%) to 76.723% when pulling **f36dd6cb0646d7eb56b64e953c7148aed994efc4 on arturobernalg:feature/inline** into **c4a62e53b1d31fd2c1b1e0367947b9cadf11ceeb on apache:master**.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] arturobernalg commented on a change in pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r665699711
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
##########
@@ -85,26 +85,24 @@ private void interpretTile(final ImageBuilder imageBuilder, final byte[] bytes,
if (sampleFormat == TiffTagConstants.SAMPLE_FORMAT_VALUE_IEEE_FLOATING_POINT) {
// tileLength: number of rows in tile
// tileWidth: number of columns in tile
- final int i0 = startY;
Review comment:
Done
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] arturobernalg commented on a change in pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r663096245
##########
File path: src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java
##########
@@ -1003,7 +1003,6 @@ public void writeImage(final BufferedImage src, final OutputStream os, Map<Strin
// boolean LocalColorTableFlag = false;
final boolean interlaceFlag = false;
final boolean sortFlag = false;
- final int sizeOfLocalColorTable = colorTableScaleLessOne;
Review comment:
HI @kinow
Changed
TY
##########
File path: src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/Dct.java
##########
@@ -282,15 +282,14 @@ public static void inverseDCT8(final float[] vector) {
final float n1 = vector[0] - vector[4];
final float n2 = b2 - a3;
final float n3 = vector[0] + vector[4];
- final float neg_n5 = neg_b4;
// A2
final float m3 = n1 + n2;
final float m4 = n3 + a3;
final float m5 = n1 - n2;
final float m6 = n3 - a3;
- // float m7 = n5 - n0;
- final float neg_m7 = neg_n5 + n0;
+ // float m7 = b4 - n0;
+ final float neg_m7 = neg_b4 + n0;
Review comment:
HI @kinow
Changed
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] arturobernalg commented on a change in pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r665699120
##########
File path: src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/JpegDecoder.java
##########
@@ -242,43 +242,41 @@ public boolean visitSegment(final int marker, final byte[] markerBytes,
} else if (marker == JpegConstants.DQT_MARKER) {
final DqtSegment dqtSegment = new DqtSegment(marker, segmentData);
for (final QuantizationTable element : dqtSegment.quantizationTables) {
- final DqtSegment.QuantizationTable table = element;
Review comment:
HI @kinow
I am agree with `table` name. Seem more logical.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r662989639
##########
File path: src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java
##########
@@ -1003,7 +1003,6 @@ public void writeImage(final BufferedImage src, final OutputStream os, Map<Strin
// boolean LocalColorTableFlag = false;
final boolean interlaceFlag = false;
final boolean sortFlag = false;
- final int sizeOfLocalColorTable = colorTableScaleLessOne;
Review comment:
I think the author here intentionally made a redundant variable so other developers would understand the code more easily. Imaging has a lot of code with many variables, or methods implementing papers and using the variables as they appear in the papers.
So any help like this helps, so IMO we should keep issues like that when found by static analyzers / linters.
##########
File path: src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/Dct.java
##########
@@ -282,15 +282,14 @@ public static void inverseDCT8(final float[] vector) {
final float n1 = vector[0] - vector[4];
final float n2 = b2 - a3;
final float n3 = vector[0] + vector[4];
- final float neg_n5 = neg_b4;
// A2
final float m3 = n1 + n2;
final float m4 = n3 + a3;
final float m5 = n1 - n2;
final float m6 = n3 - a3;
- // float m7 = n5 - n0;
- final float neg_m7 = neg_n5 + n0;
+ // float m7 = b4 - n0;
+ final float neg_m7 = neg_b4 + n0;
Review comment:
Quite sure these variables were copied from a paper or specification document. I would prefer to leave as in the document (if that's the case), but will have to take a better look later to verify if that's really the case.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r706913932
##########
File path: src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/JpegDecoder.java
##########
@@ -262,23 +261,22 @@ public boolean visitSegment(final int marker, final byte[] markerBytes,
} else if (marker == JpegConstants.DHT_MARKER) {
final DhtSegment dhtSegment = new DhtSegment(marker, segmentData);
for (final HuffmanTable element : dhtSegment.huffmanTables) {
Review comment:
Sorry, came to review this one today, and realized I had an old comment pending to be submitted.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow closed pull request #155: [IMAGING-315] Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
kinow closed pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r665701393
##########
File path: src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/JpegDecoder.java
##########
@@ -262,23 +261,22 @@ public boolean visitSegment(final int marker, final byte[] markerBytes,
} else if (marker == JpegConstants.DHT_MARKER) {
final DhtSegment dhtSegment = new DhtSegment(marker, segmentData);
for (final HuffmanTable element : dhtSegment.huffmanTables) {
Review comment:
We can rename this one to `table` too @arturobernalg
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] coveralls commented on pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#issuecomment-861667397
[](https://coveralls.io/builds/40605270)
Coverage decreased (-0.01%) to 76.654% when pulling **194283235697b5bc0a933bff33e255a3a9eb767a on arturobernalg:feature/inline** into **471475c860a6106684ce33cbda96d20d03504c86 on apache:master**.
--
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-imaging] kinow commented on a change in pull request #155: Remove redundant local variable.
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r663321550
##########
File path: src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/JpegDecoder.java
##########
@@ -242,43 +242,41 @@ public boolean visitSegment(final int marker, final byte[] markerBytes,
} else if (marker == JpegConstants.DQT_MARKER) {
final DqtSegment dqtSegment = new DqtSegment(marker, segmentData);
for (final QuantizationTable element : dqtSegment.quantizationTables) {
- final DqtSegment.QuantizationTable table = element;
Review comment:
Or we could rename `element` to `table`? WDYT @arturobernalg ? This way the "Invalid quantization table identifier" message would clearly reflect that the identifier is from `table` (as in the variable name too).
Table is also a common term in many image processing documents (I guess that's why the author of the code created the alias-variable).
##########
File path: src/main/java/org/apache/commons/imaging/formats/rgbe/RgbeImageParser.java
##########
@@ -102,15 +102,14 @@ public BufferedImage getBufferedImage(final ByteSource byteSource, final Map<Str
final DataBuffer buffer = new DataBufferFloat(info.getPixelData(),
info.getWidth() * info.getHeight());
- final BufferedImage ret = new BufferedImage(new ComponentColorModel(
+ return new BufferedImage(new ComponentColorModel(
ColorSpace.getInstance(ColorSpace.CS_sRGB), false, false,
Transparency.OPAQUE, buffer.getDataType()),
Raster.createWritableRaster(
new BandedSampleModel(buffer.getDataType(),
info.getWidth(), info.getHeight(), 3),
buffer,
new Point()), false, null);
- return ret;
Review comment:
:+1:
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
##########
@@ -85,26 +85,24 @@ private void interpretTile(final ImageBuilder imageBuilder, final byte[] bytes,
if (sampleFormat == TiffTagConstants.SAMPLE_FORMAT_VALUE_IEEE_FLOATING_POINT) {
// tileLength: number of rows in tile
// tileWidth: number of columns in tile
- final int i0 = startY;
Review comment:
Didn't search any specification document, but I would probably leave these variables as-is as they appear to be the type of vars copied from some existing code or document...
##########
File path: src/main/java/org/apache/commons/imaging/formats/png/PngImageParser.java
##########
@@ -254,9 +254,8 @@ public void readSignature(final InputStream is) throws ImageReadException,
}
final PngChunkIccp pngChunkiCCP = (PngChunkIccp) chunks.get(0);
- final byte[] bytes = pngChunkiCCP.getUncompressedProfile(); // TODO should this be a clone?
- return (bytes);
+ return (pngChunkiCCP.getUncompressedProfile());
Review comment:
This removes the `TODO` comment that was left there by Sebb during one of his reviews. Better leave it there until someone else has time to ponder on that item, and maybe remove the redundant variable.
##########
File path: src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/JpegDecoder.java
##########
@@ -376,16 +374,14 @@ private void readMCU(final JpegInputStream is, final int[] preds, final Block[]
is,
huffmanACTables[scanComponent.acCodingTableSelector]);
final int ssss = rs & 0xf;
- final int rrrr = rs >> 4;
- final int r = rrrr;
Review comment:
It does look odd, but if you look at the comments near this code, they are pointing to which page of a document the author was referencing when writing it. These variable names, though obscure or redundant, are more helpful for devs familiar with that document.
See this Python examples:
- https://github.com/Exaphis/python-jpeg-decoder/blob/849c3d0bb32355e44a40364ea0498e6f59443716/jpeg.py#L302
- https://github.com/yohhoy/picojdec/blob/316ef275c757bb67d4398980660f42915f67901c/picojdec.py#L152
Or these excerpts from a specification document:
I use Eclipse + IntelliJ (and other tools like CPD, SpotBugs, stan4j, sonarqube, etc) but for [imaging] I ignore several issues to avoid reducing the clarity of the code for future developers familiar with image processing :+1:
So sorry for turning down changes like this @arturobernalg , even though you are correct some variables are redundant.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org