Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Fix conversion of ip address to int #199

Merged
merged 2 commits into from
Jun 20, 2017

Conversation

mabn
Copy link
Contributor

@mabn mabn commented Jun 19, 2017

Properly convert bytes to unsigned integers

Fixes #198

Properly convert bytes to unsigned integers
@codecov-io
Copy link

codecov-io commented Jun 19, 2017

Codecov Report

Merging #199 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #199   +/-   ##
=========================================
  Coverage     81.34%   81.34%           
  Complexity      470      470           
=========================================
  Files            77       77           
  Lines          1812     1812           
  Branches        214      214           
=========================================
  Hits           1474     1474           
  Misses          249      249           
  Partials         89       89
Impacted Files Coverage Δ Complexity Δ
...ore/src/main/java/com/uber/jaeger/utils/Utils.java 78.94% <100%> (ø) 6 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74faa98...f3abc85. Read the comment docs.

@@ -50,7 +50,7 @@ public static int ipToInt(String ip) throws EmptyIpException, NotFourOctetsExcep

int intIp = 0;
for (byte octet : octets.getAddress()) {
intIp = (intIp << 8) | (octet);
intIp = (intIp << 8) | (octet & 0xFF);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my education, since octet is already of type byte, why does & 0xFF make a difference?

Copy link
Contributor Author

@mabn mabn Jun 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

octet is promoted to integer via binary numeric promotion (https://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.6.2), but it's a signed integer so the mask is filled with 1s on the left for negative numbers. & 0xFF converts it to unsigned (It's a byte so no need to care about int overflow. If it mattered then it should be 0xFFL). At least that's how I understand that.

In Java 8 that would instead be: Byte.toUnsignedInt(octet)


@Test
public void testIpToInt32_above127() {
int expectedIp = 176750850;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the magic number is hard to validate, how about a sanity check

assertEquals(expectedIp, (10 << 24) | (137 << 16) | (1 << 8) | 2); // sanity check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I've also changed assertions to assertThat(x, equals(y)), I think that they are more readable

@yurishkuro yurishkuro merged commit a4ce2b0 into jaegertracing:master Jun 20, 2017
@yurishkuro
Copy link
Member

Thanks!

@mabn mabn deleted the fix_ip_to_int branch June 20, 2017 19:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants