-
Notifications
You must be signed in to change notification settings - Fork 357
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 net.TCPAddr to extract remote IP address #101
Conversation
if err != nil { | ||
return nil, err | ||
} | ||
remoteAddr := tconn.RemoteAddr().(*net.TCPAddr) |
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.
What it the conversion fails ?
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.
It can never fail, because we do net.DialTimeout("tcp", ...)
before and thus it is always a TCPAddr
.
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.
Well, it is not guaranteed at compile time, so I'd prefer to have a check so it can never crash at runtime.
Even if it should never happen.
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.
If the check fails something must be terribly wrong. I would like to see the program panicing in that case. Do you accept a panic("type assertion failed")
?
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.
@jlaffaye ping
Well, what's the benefits of this change ? |
It avoids unnecessary conversion to string and parsing the string. As I already mentioned: It can never be anything than |
In case this behaviour ever changes in a future version of Go, our tests will indicate that. |
thanks for merging! |
No description provided.