Remove a lot of unwanted ifdef'd out code#826
Remove a lot of unwanted ifdef'd out code#826kinkie wants to merge 96 commits intosquid-cache:masterfrom
Conversation
squid-anubis
added
M-failed-description
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Thank you for cleaning this up!
What is the exact scope of this PR? AFAICT, it is 0 and SILLY_CODE which feels rather odd. Why not add UNUSED_CODE? There is also UNREACHABLECODE to consider (among other candidates). Please explain what code this PR removes in the PR description. We cannot hope to remove every code snippet that should be removed, of course, but we should be clear about what we are removing.
src/adaptation/ecap/XactionRep.cc:#if 0 /* XXX: implement */
Also, please double check that you did not miss any 0 cases (or explicitly mention in the PR description that you left some in on purpose). AFAICT, some are still present, but I did not check carefully.
|
RFC1035_UNPACK_DEBUG is defined as (void)0. This has been off forever,
the intent seems quite clear to me
On Sat, 15 May 2021 at 15:33, Amos Jeffries ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/dns/rfc1035.cc
<#826 (comment)>:
> @@ -251,53 +245,40 @@ rfc1035NameUnpack(const char *buf, size_t sz, unsigned int *off, unsigned short
size_t len;
assert(ns > 0);
do {
- if ((*off) >= sz) {
- RFC1035_UNPACK_DEBUG;
These should probably be replaced with debugs().
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#826 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHPVDACIMFTFYIEMP5NG7LTNZZ2HANCNFSM445ZKEZQ>
.
--
@mobile
|
I started with SILLY_CODE and if 0.
I left behind about 5 intentionally. Those I left behind, I did because I had the impression that they served an useful purpose, maybe as documentation, maybe it an attempted and aborted implementation that might be still taken up and would be wasteful to restart from scratch. |
FYI, I turned it on privately to test the EDNS packets arriving from various DNS servers when that feature went in and first bugs found. Its just not been turned on publicly for everyone (yet). |
|
Ok, I'll turn then into level 8 debugs()
…On Sun, 16 May 2021, 03:32 Amos Jeffries, ***@***.***> wrote:
RFC1035_UNPACK_DEBUG is defined as (void)0. This has been off forever, the
intent seems quite clear to me
FYI, I turned it on privately to test the EDNS packets arriving from
various DNS servers when that feature went in and first bugs found. Its
just not been turned on publicly for everyone (yet).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#826 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHPVDAIE3C7XWWUFSCXSSDTN4ODDANCNFSM445ZKEZQ>
.
|
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
kinkie requested a review from rousskov 4 hours ago
If you recall, I suggested changes to the PR description (i.e. the future commit message). It looks like you have not changed it. Do you want me to edit the description to reflect (my understanding of) your response? Or perhaps this review request was just for the existing code changes, and you plan to work on the description later?
I can also go after UNUSED_CODE and UNREACHABLECODE.
Thank you. How about LINGERING_CLOSE that is marked (in comm.cc) as "broken, unused, and untested since 933dd09"? I know that code wasted quite a bit of time in relatively recent developments. IIRC, it cannot even compile since that ten-year old commit.
I would also look at #if SIZEOF_INT64_T == 4 for (hopefully) obvious reasons. If that can be removed, that would remove the whole clientHttpRequestStatus() function and quite a bit of calling code, right?
|
On Mon, May 17, 2021 at 8:39 PM Alex Rousskov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/dns/rfc1035.cc
<#826 (comment)>:
> @@ -252,50 +248,46 @@ rfc1035NameUnpack(const char *buf, size_t sz, unsigned int *off, unsigned short
assert(ns > 0);
do {
if ((*off) >= sz) {
- RFC1035_UNPACK_DEBUG;
+ debugs(DEBUG_SECTION, DBG_DATA, "unpack error: parsing beyond end");
Alex: Please explain what code this PR removes in the PR description.
Alex: please ... explicitly mention in the PR description that you left
some in on purpose.
I had not interpreted your commment as advice to change the description.
Sigh. I really do not know how I could have been any more explicit/direct
in my change request. What should I say the next time to avoid this kind of
misunderstanding?
I don't recall noticing that comment and when I went over the comments
again on your second iteration I could not find it.
I am sorry; I must be github-blind for some reason.
I think you went ahead, thanks for doing that.
I did not (you can see PR description editing history by clicking the
"edited" link above the description box). Do you want me to? Needless to
say, my scope definition may not match how you see your changes (but you
can adjust it later, of course).
Feel free to, please.
I will iterate if I have any issues with your proposal
…--
Francesco
|
FYI: The comment came from the review heading/description at #826 (review) which was explicitly linked from the next iteration text as well. You should also have an email notification from GitHub with that same text. You did read it because you responded to (and even quoted) some of the suggestions in my text. Searching this PR page in a browser for "PR description" works as well (for me). |
yadij
left a comment
yadij
left a comment
There was a problem hiding this comment.
Some whitespace polish needed.
I missed the fact that some of the #if 0 being removed are in the third-party libraries we ship in compat/, lib/, and includes/. Those are there to ease future sync with third-party changes from the original source source, or security patching we may have to apply.
squid-anubis
added
the
M-failed-description
|
Other #ifdefd-code I could consider candidates for removal but would : ADD_X_REQUEST_URI - conditionally add a header for pconn debugging There's a few more that I think serve some purpose as comments or work in progress although there are a lot that haven't changed in a lot of time |
@yadij, please note that @kinkie has responded to your request but his response is currently inside the PR description:
FWIW, I will accept whatever decision you two can agree on here. I just did not want Amos to miss Francesco's response due to its odd location. I will edit the PR description when doing the review, per Francesco's earlier request. |
@yadij , any input from you on this? |
|
I think I have addressed all outsdanding comments except for the one on third-party libraries being left alone. I'm requesting a review to get this moving again |
kinkie
added
the
S-waiting-for-reviewer
The compat/ and third-party lib/ are by intention code we should not be touching regularly. So years between touches is not a sign of ability to remove code there. The situations I mentioned are ones which have occured. Though admittedly years ago when I did an audit to see how what we could drop completely or upgrade from ancient code to newer third-party releases. smblib and rfcnb libs got security patches and upgrades respectively. |
Also add comment for future work
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
That file is being reworked as part of PR squid-cache#827
|
@rousskov , any objections to merging? |
I still need to adjust the PR description and review the PR itself. FYI: I am doing my best to follow a certain unofficial PR review order (subject to |
|
No need to rush or override.
Thanks for explaining your process
On Fri, 21 May 2021 at 17:51, Alex Rousskov ***@***.***> wrote:
rousskov, any objections to merging?
I still need to adjust the PR description and review the PR itself.
FYI: I am doing my best to follow a certain unofficial PR review order
<https://github.com/measurement-factory/squid-notes/blob/start/review.md#pull-request-review-order>
(subject to S-waiting-for-reviewer labels in PRs authored by folks who
have permissions to set labels). As a PR author with core developer rights,
you can partially control that order by using review-* labels (as
documented on that linked page).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#826 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHPVDAHOOIOG6X5FWL72FLTOZ6QPANCNFSM445ZKEZQ>
.
--
@mobile
|
rousskov
changed the title
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
I suggest removing NOT_IMPLEMENTED as well, but I do not insist on that removal.
I changed PR title/description to better reflect the scope of this PR. Please adjust further if needed.
Please clear for merge when ready -- no need for another review round AFAICT. The remaining changes are very minor/simple (and only one -- non_get -- is required)
rousskov
added
S-waiting-for-author
|
Commit c4335a6 is for me a candidate for merge, according to Alex' comment . I'll wait until tomorrow (CEST) for any last-minute requests |
rousskov
dismissed
their stale review
The change requests in this old review were addressed and/or superseded by my no-objection comment
rousskov
added
S-waiting-for-committer
We examined most `#if...` clauses and removed those we decided are clearly unwanted now. Most of the removed code snippets were unused for many years or represented stale ideas. We tried to check (at least superficially) all clauses, but most likely missed some good candidates.
squid-anubis
added
the
M-waiting-staging-checks
yadij
added
M-cleared-for-merge
squid-anubis
added
M-merged
4 participants
We examined most
#if...clauses and removed those we decided areclearly unwanted now. Most of the removed code snippets were unused for
many years or represented stale ideas. We tried to check (at least
superficially) all clauses, but most likely missed some good candidates.