Skip to content
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

Maintenance: Remove change-minimizing MemObject::Io hack #1957

Conversation

vshailesh
Copy link
Contributor

@vshailesh vshailesh commented Dec 3, 2024

Added in 2018 commit 4310f8b.

src/MemObject.h Outdated Show resolved Hide resolved
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Dec 3, 2024
@rousskov rousskov changed the title Completed Todo : Remove change minimizing code Maintenance: Remove change-minimizing MemObject::Io hack Dec 3, 2024
@rousskov
Copy link
Contributor

rousskov commented Dec 3, 2024

I have adjusted PR title to be more specific and use Maintenance: prefix that tells us that this change does not affect Squid behavior in any way (and, hence, is safe to backport, skip during bug-searching bisection, etc.). @vshailesh, please keep this change in mind when naming future code maintenance PRs.

I also adjusted PR description to supply useful information instead of narrating arguably self-documenting diff.

…specifiers, instead using enum values directly from Store class
…specifiers, instead using enum values directly from Store class
@vshailesh
Copy link
Contributor Author

@rousskov I have made the changes in all the files that were using MemObject::io****.

Please review the changes

@vshailesh vshailesh requested a review from rousskov December 13, 2024 12:12
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for completing this TODO!

P.S. Just FYI: You do not have to use formal prefixes like Maintenance: in individual branch commit titles, especially branch commits other than the first one1. There is nothing wrong with using them there either, but my earlier comment about prefixes only applied to PR title (that becomes official commit message title when PR is auto-merged by Anubis).

Footnotes

  1. Various PR creation tools like gh treat "single-commit PR branches" specially, automatically using that single commit message to compute PR title/description.

@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-author author action is expected (and usually required) labels Dec 13, 2024
squid-anubis pushed a commit that referenced this pull request Dec 13, 2024
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Dec 13, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants