[squid-dev] RFC: Modernizing sources using clang-tidy

[Alex Rousskov]

On 5/30/20 3:22 PM, Amos Jeffries wrote:
> On 20/04/20 2:02 pm, Alex Rousskov wrote:
>>     Squid sources contain a lot of poorly written, obsolete, and
>> inconsistent code that (objectively) complicates development and
>> (unfortunately) increases tensions among developers during review.
>>
>> Some of those problems can be solved using tools that modify sources.
>> Clang-tidy is one such tool: https://clang.llvm.org/extra/clang-tidy/
>> It contains 150+ "checks" that can automatically fix some common
>> problems by studying the syntax tree produced by the clang compiler.


> In order to switch we should be looking for a tool that improves over
> the status-quo. Which is both astyle plus the custom scripts.
> 
> All of the above are high-level abstractions that the existing astyle
> tool provides by itself. So no valid reason for changing is visible yet.

Amos, you are mixing up two completely different subjects: modernizing
code using clang-tidy (this thread) and formatting sources using
clang-format (Francesco's effort). In hope to make progress, I am
ignoring most comments about astyle.


>> https://clang.llvm.org/extra/clang-tidy/checks/list.html

>> * performance-...
>>
>> Clang-tidy has a few checks focusing on performance optimizations. The
>> following commit shows a combination of the following four checks:
>> performance-trivially-destructible, performance-unnecessary-value-param,
>> performance-for-range-copy, performance-move-const-arg
>>
>> Diff: https://github.com/measurement-factory/squid/commit/1ae5d7c


> IMO this is something we should have a

Sorry, was this phrase cut prematurely or are you voicing support (in
principle) for applying (some of these) performance-related changes?


>> * modernize-use-nullptr
>>
>> Replaces NULL and 0 constants with C++11 nullptr:
>> https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html
>>
>> While replacing most NULLs is possible with a simple script, this check
>> may be a better solution because it can safely cover more NULL uses and
>> also converts bare 0s which would be impossible using a script.
>>
>> Diff: https://github.com/measurement-factory/squid/commit/4242604

> As I mentioned earlier when kinkie tried this; I do not like this
> particular tool feature. The "upgrade" it does is far too simplistic for
> a style polishing update. If the end goal were simply to eradicate NULL
> - this would be fine. But the goal of using this tool is to ensure a
> good quality formatting style.

... except it is not the goal. modernize-use-nullptr does not change
code formatting.


> A simplistic s/NULL/nullptr/ leaves quite
> a few code lines looking just as ugly as they were with NULL.

And, as I mentioned above, modernize-use-nullptr is not simplistic --
nothing else can detect and replace 0s with nullptr.


> IMO we would be better off going the scripted way to remove the subset
> of cases we can automate and catch the rest with manual edits and in review.

I do not see a point of automating ourselves if there is an existing
automation that works much better than anything we can do ourselves.


> I like a few things the tool does. But so far it seems like something we
> want to run across the code occasionally. eg as a Jenkins test job.

Yes, or a Semaphore CI job, and/or on-demand. Please clarify regarding
performance-* checks above. If we are in agreement regarding
modernize-use-override and at least one more check, then I can start
working on a polished proposal for those checks (at least) and the
overall setup.


Cheers,

Alex.