Maintenance: Removed deprecated Client::replyBodySpace()#2060
Closed
eduard-bagdasaryan wants to merge 2 commits into
Closed
Conversation
The deprecated method was basically a duplicate of Client::calcBufferSpaceToReserve().
rousskov
approved these changes
May 8, 2025
| const int read_sz = replyBodySpace(*data.readBuf, 0); | ||
| // XXX: We only use this call to decide whether to read; we never increase data.readBuf space. | ||
| // TODO: Upgrade data.readBuf to SBuf and merge this with similar HttpStateData::readReply() code. | ||
| const auto read_sz = calcBufferSpaceToReserve(data.readBuf->spaceSize(), data.readBuf->spaceSize()); |
Contributor
There was a problem hiding this comment.
- The second parameter in removed replyBodySpace() is called minSpace. We passed 0.
- The second parameter in calcBufferSpaceToReserve() is called wantSpace. We now pass spaceSize().
In both cases, the end result is exactly the same!
The value change makes sense because parameter names differ, and buffer spaceSize() is much closer to wantSpace than 0 would be -- we do not want to get rid of the existing buffer space. Neither value is actually "ideal" or "best", but improving requires difficult refactoring that lies outside this no-behavior-changes cleanup PR. We have several related TODOs/XXXs and runtime bugs. This small/simple PR helps with ongoing refactoring that will eventually address those. It is a small but useful step forward.
This comment does not request any PR changes.
rousskov
changed the title
rousskov
added
M-cleared-for-merge
Contributor
|
There is also the change of |
Contributor
Yes, I have adjusted PR description to document that difference as well. The different code is never executed for the removed call and for the added one (because the corresponding |
kinkie
approved these changes
May 9, 2025
squid-anubis
pushed a commit
that referenced
this pull request
May 10, 2025
Except for a trivial spaceSize() call and a never-executed (for the only caller) `space` parameter adjustment, replyBodySpace() duplicates Client::calcBufferSpaceToReserve(). This duplication complicates refactoring aimed at addressing other known buffer management problems.
squid-anubis
added
M-waiting-staging-checks
kinkie
removed
M-failed-staging-checks
squid-anubis
pushed a commit
that referenced
this pull request
May 11, 2025
Except for a trivial spaceSize() call and a never-executed (for the only caller) `space` parameter adjustment, replyBodySpace() duplicates Client::calcBufferSpaceToReserve(). This duplication complicates refactoring aimed at addressing other known buffer management problems.
squid-anubis
added
M-waiting-staging-checks
rousskov
removed
M-failed-staging-checks
squid-anubis
pushed a commit
that referenced
this pull request
May 11, 2025
Except for a trivial spaceSize() call and a never-executed (for the only caller) `space` parameter adjustment, replyBodySpace() duplicates Client::calcBufferSpaceToReserve(). That duplication complicates refactoring aimed at addressing other known buffer management problems.
squid-anubis
added
M-waiting-staging-checks
rousskov
removed
the
M-failed-staging-checks
squid-anubis
pushed a commit
that referenced
this pull request
May 12, 2025
Except for a trivial spaceSize() call and a never-executed (for the only caller) `space` parameter adjustment, replyBodySpace() duplicates Client::calcBufferSpaceToReserve(). This duplication complicates refactoring aimed at addressing other known buffer management problems.
squid-anubis
added
M-waiting-staging-checks
Contributor
|
@eduard-bagdasaryan, our issues with github have now been resolved. You just need to rebase/merge the PR branch on latest master branch content (or click the "update branch" button on github UI that does it for you). |
yadij
added
the
S-waiting-for-author
Contributor
Author
|
Done, please remove the M-failed-staging-checks label to get it merged. |
rousskov
removed
S-waiting-for-author
squid-anubis
pushed a commit
that referenced
this pull request
May 19, 2025
Except for a trivial spaceSize() call and a never-executed (for the only caller) `space` parameter adjustment, replyBodySpace() duplicates Client::calcBufferSpaceToReserve(). This duplication complicates refactoring aimed at addressing other known buffer management problems.
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Except for a trivial spaceSize() call and a never-executed (for the only
caller)
spaceparameter adjustment, replyBodySpace() duplicatesClient::calcBufferSpaceToReserve(). This duplication complicates
refactoring aimed at addressing other known buffer management problems.