Refactor Mgr::StringParam.str to be a SBuf [DRAFT]#2405
Refactor Mgr::StringParam.str to be a SBuf [DRAFT]#2405kinkie wants to merge 1 commit intosquid-cache:masterfrom
Conversation
|
This was run-tested with active workers on Linux. pre-staging test at https://github.com/kinkie/squid/actions/runs/24265673016 |
yadij
left a comment
yadij
left a comment
There was a problem hiding this comment.
Please replace the existing Ipc::TypedMsgHdr::getString() to work based on the SBuf member instead of what it currently does with char[].
| static SBuf t; | ||
| const SBuf& Mgr::StringParam::value() const STUB_RETVAL(t) |
There was a problem hiding this comment.
We no longer need a separate static declaration.
| static SBuf t; | |
| const SBuf& Mgr::StringParam::value() const STUB_RETVAL(t) | |
| const SBuf& Mgr::StringParam::value() const STUB_RETREF(SBuf) |
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Thank you for improving this code.
| // SBuf doesn't need special handling for empty | ||
| if (!length) { | ||
| s.clear(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The if statement contradicts its comment/description. Please either remove the comment or remove both the comment and the statement. I do recommend removing.
| // SBuf doesn't need special handling for empty | |
| if (!length) { | |
| s.clear(); | |
| return; | |
| } | |
| if (!length) { | |
| s.clear(); | |
| return; | |
| } |
or
| // SBuf doesn't need special handling for empty | |
| if (!length) { | |
| s.clear(); | |
| return; | |
| } |
or
| // SBuf doesn't need special handling for empty | |
| if (!length) { | |
| s.clear(); | |
| return; | |
| } | |
| // zero length OK |
| { | ||
| const int length = getInt(); | ||
| Must(length >= 0); | ||
| // String uses memcpy uncoditionally; TODO: SBuf eliminates this check |
There was a problem hiding this comment.
Please remove this stale TODO:
- // String uses memcpy uncoditionally| } | ||
|
|
||
| Must(length <= maxSize); | ||
| // TODO: use SBuf.reserve() instead of a temporary buffer |
There was a problem hiding this comment.
Please remove this stale TODO:
- // TODO: use SBuf.reserve() instead of a temporary buffer| Ipc::TypedMsgHdr::getSBuf(SBuf &s) const | ||
| { | ||
| const int length = getInt(); | ||
| Must(length >= 0); |
There was a problem hiding this comment.
Here and elsewhere, please avoid deprecated Must() in new code. Throw a TextException instead.
| char buf[maxSize]; | ||
| getRaw(&buf, length); | ||
| s.assign(buf, length); |
There was a problem hiding this comment.
Amos at #2405 (review): Please replace the existing
Ipc::TypedMsgHdr::getString()to work based on theSBufmember instead of what it currently does withchar[].
Please do not change this code in this PR. We do not want to add a temporary heap allocation here. When String users of TypedMsgHdr are gone, this code will be removed.
P.S. There is no "SBuf member" in Ipc::TypedMsgHdr and there should not be any (in the foreseeable future). I assume the quoted suggestion meant to say "local variable" instead of "member", but if my assumption is wrong, then please do not add an SBuf member to Ipc::TypedMsgHdr!
| void getSBuf(SBuf &s) const; ///< load variable-length SBuf | ||
| void putSBuf(const SBuf &s); ///< store variable-length SBuf |
There was a problem hiding this comment.
I recommend overloading these methods. They are not about the exact storage type (which may change). They are about extracting a {length, data} field.
| void getSBuf(SBuf &s) const; ///< load variable-length SBuf | |
| void putSBuf(const SBuf &s); ///< store variable-length SBuf | |
| void getString(SBuf &) const; ///< load a variable-length field | |
| void putString(const SBuf &); ///< store a variable-length field |
Overloading would also reduce noise when callers are upgraded to use SBuf.
rousskov
added
the
S-waiting-for-author
3 participants
No description provided.