squid
squid copied to clipboard
squid-cache
Maintenance: Automatic upgrade of NULL to nullptr in C++ code
Maintenance script to enforce use of nullptr in C++ code.
NULL is still valid in C code, and supported by C++. nullptr is just a nicer construct for C++ use with extra type safety and compiler optimizations possible.
Depends on PR #531 to have any effect on the code.
As of commit 7b07896 these remove just over 40% of existing NULL.
I am very concerned that applying this script the first time will change a lot of files but still will not solve the NULL problem. I hope we can do a lot better. I am currently checking one alternative solution and will be back once I have tested it.
I am interested in what you consider a "final" solution given that NULL is still a valid construct. As such it can be included by any third-party header file, macro, or template. No solution that defines or undef's NULL to cause a compiler error/warning will work - I looked into those already long ago.
I also have one specific question: Why three scripts instead of just one? Is this proliferation due to some kind of awk limitation?
Each script contains the rules to perform a different type of operational change.
I am interested in what you consider a "final" solution
I hope to post my suggestions this week.
Each script contains the rules to perform a different type of operational change.
I do not think the current differences among the "type of operational change" deserve introduction of type-specific plugins. They probably deserve a source code comment above the group of type-specific substitutions, but a separate plugin feels like an overkill (which creates more problems than it solves).
If we end up using this overall approach, we should merge the three plugins into one IMO, but I am not asking for that change at this time because I hope that this plugin will not be needed at all.
clang-tidy has a quite effective modernize-use-nullptr module for this purpose. It seems to be working well except in a handful of corner cases that look funny but compile and pass the unit tests. I'll post a sample branch in a few hours
Please see https://github.com/kinkie/squid/tree/nullptr : 454 files changed, 4056 insertions(+), 4056 deletions(-)
Please see https://github.com/kinkie/squid/tree/nullptr : 454 files changed, 4056 insertions(+), 4056 deletions(-)
This is effectively just a s/NULL/nullptr/ with better detection of what is C++ vs C sources. I could have done that with a three-line awk rule. But many of the current NULL are embedded in lines which need better conversion than just replace. Some need reordering of the line keywords, use of operator !() and such for conditionals for actually improved C++ syntax. Thus the more specific and incremental replacements.
Yes. I thought of that. I agree with you that we want to do more than just search and replace NULL with nullptr. A tool that understand the AST can do it better than a hand rolled, for instance we have NULLs in comments. I could look into writing a clang-tidy module to do the extra bits, I wonder if it’s worth the effort.
On Sun, 19 Apr 2020 at 14:57, Amos Jeffries [email protected] wrote:
Please see https://github.com/kinkie/squid/tree/nullptr : 454 files changed, 4056 insertions(+), 4056 deletions(-)
This is effectively just a s/NULL/nullptr/ with better detection of what is C++ vs C sources. I could have done that with a three-line awk rule. But many of the current NULL are embedded in lines which need better conversion than just replace. Some need reordering of the line keywords, use of operator !() and such for conditionals for actually improved C++ syntax. Thus the more specific and incremental replacements.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/squid-cache/squid/pull/598#issuecomment-616141011, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHPVDF5JQRWVUEHSTYH4WTRNL7MBANCNFSM4MGJKARQ .
-- @mobile
clang-tidy has a quite effective modernize-use-nullptr module for this purpose
It was also a part of my squid-dev RFC.
This is effectively just a s/NULL/nullptr/ with better detection of what is C++ vs C sources
It is a lot more than that as I detailed in my RFC, but it does not really matter here -- s/NULL/nullptr/ with exclusion of C sources would suffice IMO.
many of the current NULL are embedded in lines which need better conversion than just replace.
I disagree with the implication of that assertion. Yes, some lines can and should be polished further, but we can keep all that (mostly human-required) polishing outside the automated changes scope. We can and, IMO, should, just get rid of NULLs, leaving other polishing to other tools/humans/times.
I could look into writing a clang-tidy module to do the extra bits, I wonder if it’s worth the effort.
IMO, a better NULL replacement is not worth your time, especially if my understanding of the difficulties involved with writing and sharing such a module is correct. If Amos withdraws the requirement of applying "30" possible polishing changes to every NULL-carrying context, then we can automatically remove most NULLs and then manually remove the few remaining NULLs missed by the tools (e.g., all NULLs in C++ comments). It is really not a big deal! What makes this NULL removal a big deal is the insistence on integrating further polishing with the initial removal.
BTW, the same arguments apply to HERE removal IMO.
I have updated the description to clarify that NULL is not a "problem" - nullptr is just nicer to have in C++ code. If there is a reason to use NULL it can be used.
The scope of this PR is to add enforcement of nullptr for the simpler cases where our style preferences can be automated. Any NULL needing complex attention is left out of scope as probably not currently worth time spent automating - they can be handled later and/or updated in QA review when touched.
After the initial formatting bump these script rules are intended for long-term use helping PR review minimize minor polish requests. If the code produced by a tool is not exactly what we want the code to look like (i.e. actively reducing review comments), then it is not worth integrating yet and out of scope for this PR.
As such; clang-tidy or similar replacement might be useful for one-off bumps just to polish sub-sections of the code in bulk. But the code is not clean enough yet for long-term use across everything.
FYI: the initial enforcement bump touches only 2% of the codebase. For comparison the HERE bump is 1% after 10 years of manual removals, and first use of astyle was 10-15%).
clang-tidy or similar replacement might be useful for one-off bumps just to polish sub-sections of the code in bulk. But the code is not clean enough yet for long-term use across everything.
I do not understand the "sub-sections" and the "But" parts. I see no significant problems with removing all NULLs from C++ code using a combination of clang-tidy (and/or a custom script) and a few manual one-time changes (where needed).
The "problem" is that a global/blanket replacement makes the not-so-nice bits of code harder to find for future cleanup work. Removal of NULL is just a "nice to have" piece of polishing, so it is really only worthwhile doing when the result is clean polished code. It is a complete waste of time integrating and maintaining complex tools to do a no-op change.
Alex: I see no significant problems with removing all NULLs from C++ code
Amos: Removal of NULL ... is really only worthwhile doing when the result is clean polished code.
I disagree. IMO, given the Squid Project idiosyncrasies, removal of any widespread, often-in-the-way-during-code-reviews stale construct like NULL or HERE is worthwhile regardless of any imperfections left in some of the post-removal code.
We should not be abusing NULL as a marker for code that needs polishing (beyond NULL removal) because lots of code using NULL does not need further polishing (to the extent any Squid code can require no polishing). It is simply a bad marker.
Amos: It is a complete waste of time integrating and maintaining complex tools to do a no-op change.
I agree! I do not recommend integrating and maintaining NULL-removal tools.
Can we unstick this PR? I see consensus that doing this work is worthwhile. Looking at the thread, there is disagreement on whether to use clang-tidy or roll our own. Quickly skimming over the discussion, clang-tidy seems to have some more consensus.
Can we converge on a technical solution?
Can we unstick this PR? ... Can we converge on a technical solution?
If nobody beats me to it, I will post a comprehensive solution based on using clang-tidy. Hopefully, that will clearly show the already known advantages of that approach and make any further debate regarding half-measures unnecessary.
Alex: If nobody beats me to it, I will post a comprehensive solution based on using clang-tidy. Hopefully, that will clearly show the already known advantages of that approach and make any further debate regarding half-measures unnecessary.
Done at #1075. FWIW, clang-tidy results mostly met my expectations. "Inaccessible" preprocessor-guarded code and comments can be used to discredit the comprehensiveness of the clang-based solution, but the results are still much better than what a conservative syntax-unaware script can do.
Rebased on current Squid-6 code. As demonstrated by ca6547c over 380 bad uses of NULL have crept back in due to absence of automatic enforcement.
rousskov removed the request for review from kinkie rousskov removed the S-waiting-for-QA label
I have removed those items from this closed PR. If they were meant to track to-dos or other activity, please create the corresponding items elsewhere. A closed PR should not wait for something.