Fix a bunch of Implicit type narrowing warnings
#4288
Conversation
This prevents an implicit type cast. The scale factor is 2 to an integer power, which is always an integer, and gets multiplied by an int pixel value with int result expected.
This makes more sense than int, since it's calculated from file positions.
This is just for drawing text on a test image, so rounding doesn't really matter and we for sure do not need sub-pixel precision.
This prevents an implicit cast later on when calculating the array offsets during tile copying.
This method gets used by FakeReader to count the number of rows in a plate based on the well name, so input should be the well row label, e.g. "A", "AC".
There was a problem hiding this comment.
The proposed changes add defensive checks in a couple of locations to prevent overflows replacing them when appropriate with FormatException with appropriate error messages.
So far no such overflow had been reported as an issue by the community and the nightly CI builds have been passing with this PR included. @melissalinkert do you have any specific functional testing in mind other than code review. Otherwise, this should be good to merge.
| @@ -139,8 +139,14 @@ public String getLetter() { | |||
| public static int alphabeticIndexCount(String index) { | |||
| int count = 0; | |||
| char[] letters = index.toCharArray(); | |||
| if (letters.length > 6) { | |||
There was a problem hiding this comment.
Seeing the only place where this API is used is
i.e. translating the well row into an index, it almost feels like this could be thrown with letters.length >3.But generally agreed with the limit chosen here as the code would have resulted in an Integer overflow
There was a problem hiding this comment.
I debated being more restrictive, but chose the limit of 6 as it's public API and there's a chance that something external is using this method differently from FakeReader.
|
I didn't have any particular functional testing in mind beyond verifying that all builds pass. I don't know of data that would actually throw exceptions on any of the new overflow checks, but it seemed like a good idea to add now rather than later. None of the writers are impacted, so conversions shouldn't need to be tested either. |
I can split up if easier to review, but just wanted to get the easy CodeQL warnings sorted out.
I wouldn't expect this to impact builds at all.