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

[Amos Jeffries]

On 20/04/20 2:02 pm, Alex Rousskov wrote:
> Hello,
> 
>     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.
> Understanding the code at that level allows clang-tidy to attack
> problems that simple scripts cannot touch.
> 
> I have not studied most of the clang-tidy checks, but did try a few
> listed at the end of this email. You can see the whole list of checks at
> https://clang.llvm.org/extra/clang-tidy/checks/list.html
> 
> 
> Here are a few pros and cons of using clang-tidy compared to our own
> custom scripts:
> 
> Pros:
> 
> * maintained and improved by others
> * can fix problems that our scripts cannot see
> * covers a few rules from C++ Core Guidelines and popular Style Guides
> * arguably less likely to accidentally screw things up than our scripts
> 

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.


What I have seen from kinkies work is a few cases of little things like
ability to fix whitespace inside () conditionals and before
function/method parameter lists. Which is something astyle has not been
able to do for us and would be difficult to script.



> Cons:
> 
> * Requires installation of clang, clang-tidy-10, bear. It is not
> difficult in a CI environment, but may be too much for occasional
> contributors.

 Likewise same issues with needing a specific astyle version. So this is
more of a non-Pro than a Con.


> 
> * Clang-tidy misses files that do not participate in a specific build
> (e.g., misses many compat/ files that are not needed for an actual
> build). Applying clang-tidy to all sources will be difficult.
> 
> * Clang-tidy misses code lines that do not participate in a specific
> build (e.g., lines inside `#if HEADERS_LOG` where HEADERS_LOG was not
> defined). Applying clang-tidy to all lines will be impractical.
> 
> * Clang-tidy would be difficult to customize or adjust (probably
> requires building clang from scratch and writing low-level AST
> manipulation code).
> 

These are all regressions. Severity varies, but IMO we will need to
solve them somehow in order to remove the astyle usage. Otherwise we
would end up this just being an additional tool on the stack - rather
than a full replacement for astyle+scripts.


> * Clang-tidy is relatively slow -- the whole repository scan takes
> approximately 15-30 minutes per rule in my limited tests. Combining
> rules speeds things up, but it may still be too slow to run during every
> PR check on the current CI hardware.
> 

The current source-maintenance takes 5-10 minutes on master today and
with the scripts/maintenance/ automating growing I expect that to increase.


> * We do not have any clang-tidy experts on the development team (AFAIK).
> 

Not much of a con, we do not exactly have an astyle expert either. The
benefit of third-party tooling is that we don't need an expert. Just
someone able to read the documentation and test config settings when we
want to update the style policy.


> 
> I will itemize a few checks that I tried. The "diff" links below show
> unpolished and partial changes introduced by the corresponding checks.
> If we decide to use clang-tidy in principle, we will need to fine-tune
> each check options (at least) to get the most out of the tool.
> 
> * modernize-use-override
> 
> Adds "override" (and removes "virtual") keywords from class declarations.
> 
> This check is very useful not just because "override" helps prevent
> difficult-to-detect bugs but because it is very difficult to transition
> to using "override" _gradually_ -- some compilers reject class
> declarations that have a mixture of with-override and without-override
> methods. Moreover, adding override keywords to old class declarations is
> rather time-consuming because it is often not obvious (to a human)
> whether the class introduces a new interface or overrides and old one.
> 
> Diff: https://github.com/measurement-factory/squid/commit/d00d0a8
> 

I like.

> 
> * 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


> 
> * 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. A simplistic s/NULL/nullptr/ leaves quite
a few code lines looking just as ugly as they were with NULL.

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.


> 
> In summary, I think investing in clang-tidy would be worth it because
> the tool can address several important problems that we would otherwise
> have to leave untreated. It would take some time to agree on a set of
> checks and then properly configure/tune each one, but I think it is
> doable. I am not sure whether these checks should be applied on each PR
> check or periodically, but we can figure it out as we go.
> 
> I am not aware of any viable alternatives.
> 
> What do you think?
> 

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.
Additional to, rather than replacement for astyle.

Amos