-
Notifications
You must be signed in to change notification settings - Fork 528
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
Upgrade Http::Stream::reply to Pointer #1855
base: master
Are you sure you want to change the base?
Conversation
This PR seems to contain spurious changes |
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.
Thank you for this upgrade.
I replaced "update" with "upgrade" in PR title because the latter feels more precise and arguably self-documents the motivation behind this PR. Please undo if you think "update" is better.
Refactor to avoid storing a raw-pointer, with missing reference-counting.
Rebased on master without ESI and related callback complications. |
Co-authored-by: Alex Rousskov <[email protected]>
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.
Thank you for advancing this PR. I am glad ESI removal also helped with this PR! I only have a few small change requests left.
@@ -377,7 +379,7 @@ Http::Stream::noteSentBodyBytes(size_t bytes) | |||
|
|||
/// \return true when If-Range specs match reply, false otherwise | |||
static bool | |||
clientIfRangeMatch(ClientHttpRequest * http, HttpReply * rep) | |||
clientIfRangeMatch(ClientHttpRequest * http, const HttpReplyPointer &reply) |
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.
Let's replace this unchecked/unwanted pointer with a reference instead of just changing its type:
clientIfRangeMatch(ClientHttpRequest * http, const HttpReplyPointer &reply) | |
clientIfRangeMatch(ClientHttpRequest * http, const HttpReply &reply) |
... and adjust the function caller and pointer user accordingly.
The above will not make existing diff larger (because all these lines have to be changed anyway) but will improve code.
@@ -387,7 +389,7 @@ clientIfRangeMatch(ClientHttpRequest * http, HttpReply * rep) | |||
|
|||
/* got an ETag? */ | |||
if (spec.tag.str) { | |||
ETag rep_tag = rep->header.getETag(Http::HdrType::ETAG); | |||
ETag rep_tag = reply->header.getETag(Http::HdrType::ETAG); |
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 possible:
ETag rep_tag = reply->header.getETag(Http::HdrType::ETAG); | |
const auto rep_tag = reply.header.getETag(Http::HdrType::ETAG); |
Otherwise:
ETag rep_tag = reply->header.getETag(Http::HdrType::ETAG); | |
auto rep_tag = reply.header.getETag(Http::HdrType::ETAG); |
The above suggestions account for another change request that replaces an unchecked pointer with a reference (but its essence is unrelated to that change request).
range_err = "If-Range match failed"; | ||
|
||
else if (!http->request->range->canonize(rep)) | ||
else if (!http->request->range->canonize(reply.getRaw())) |
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 this is the only canonize() caller that supplies a pointer, please upgrade the corresponding canonize() method to take a reference instead of a pointer. The method already asserts that the pointer is not nil.
else if (!http->request->range->canonize(reply.getRaw())) | |
else if (!http->request->range->canonize(*reply)) |
Refactor to avoid storing a raw-pointer, with
missing reference-counting.