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
Remove BufStreamHandle #1433
Remove BufStreamHandle #1433
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1433 +/- ##
==========================================
+ Coverage 83.70% 84.44% +0.75%
==========================================
Files 167 166 -1
Lines 15867 15689 -178
==========================================
- Hits 13280 13248 -32
+ Misses 2587 2441 -146 |
dcd7aaa
to
e12b107
Compare
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.
These are really good changes, and thanks for keeping them split into clean commits!
Some style nits follow. One more structural question: I've never really understood why BufDnsStreamHandle
needs to carry around the remote_addr
when its send()
method takes a SerialMessage
which already includes a socket address. I feel like we should maybe kill SerialMessage
in favor of explicitly returning both the address and the underlying Message
where useful. In the current state, it's possible for the address carried around in the SerialMessage
to be out of sync with the actual handle/connection used to send it, which is just weird. (This could definitely be addressed in a later PR, just wanted to mention it while I was reviewing your work in this general area.)
Yeah, I was looking at removing the I will make the other changes you've pointed out and look a little more at the |
Ok, so I don't see a simple way to remove the name_server field without some significant changes, that are more than I want here. |
LGTM. |
eaaeb73
to
0757887
Compare
@djc, I'm just doing a little cleanup. I think there are a few todos in here, will move from draft once I change those.