Maintenance: convert ICP HttpRequest* to smart Pointer #1804
Conversation
| @@ -136,19 +136,16 @@ icp_common_t::getOpCode() const | |||
|
|
|||
| /* ICPState */ | |||
|
|
|||
| ICPState::ICPState(icp_common_t &aHeader, HttpRequest *aRequest): | |||
| ICPState::ICPState(icp_common_t &aHeader, const HttpRequestPointer &aRequest): | |||
There was a problem hiding this comment.
If you recall from previous discussions, I think we should use Foo::Pointer1 whenever that name has to be visible (for other valid reasons).
This suggestion illustrates the change but this change request applies to most other (if not all other) PR changes as well.
| ICPState::ICPState(icp_common_t &aHeader, const HttpRequestPointer &aRequest): | |
| ICPState::ICPState(icp_common_t &aHeader, const HttpRequest::Pointer &aRequest): |
I do not insist on these changes.
Footnotes
-
Instead of a FooPointer forward declaration. ↩
|
|
||
| /// \ingroup ServerProtocolICPAPI | ||
| bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request); | ||
| bool icpAccessAllowed(Ip::Address &, const HttpRequestPointer &); |
There was a problem hiding this comment.
Please keep the first parameter name. It is very useful to a human reader of this declaration.
| bool icpAccessAllowed(Ip::Address &, const HttpRequestPointer &); | |
| bool icpAccessAllowed(Ip::Address &from, const HttpRequestPointer &); |
| { | ||
| int reason = refreshCheck(entry, request, 30); | ||
| int reason = refreshCheck(entry, request.getRaw(), 30); |
There was a problem hiding this comment.
| int reason = refreshCheck(entry, request.getRaw(), 30); | |
| const auto reason = refreshCheck(entry, request.getRaw(), 30); |
| ++ refreshCounts[rcICP].total; | ||
| ++ refreshCounts[rcICP].status[reason]; | ||
| return (reason < 200) ? 0 : 1; | ||
| return !(reason < 200); |
There was a problem hiding this comment.
If you are going to make this condition look different from its duplicates (in refresh.cc context), then please simplify it:
| return !(reason < 200); | |
| return reason >= 200; |
Alternatively, keep the original look-and-feel (until all such conditions are fixed in refresh.cc):
| return !(reason < 200); | |
| return (reason < 200) ? false : true; |
| @@ -536,8 +529,6 @@ doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header) | |||
| } | |||
|
|
|||
| icpCreateAndSend(codeToSend, flags, url, header.reqnum, src_rtt, fd, from, state.al); | |||
There was a problem hiding this comment.
I recommend removing "Maintenance: " from PR title because these changes might (positively) affect current or future code if, for example, icpCreateAndSend() throws. I do not insist on this change.
rousskov
added
the
S-waiting-for-author
squid-anubis
added
M-failed-other
No description provided.