In this post, I report on the clang-tidy checks I still hadn’t tried as of last post.
It wasn’t pretty. Spoiler alert: trying them ultimately resulted in filing three bugs in the clang-tidy tracker. The final summary is as follows: the files column contains the number of files touched, the lines column contains the number of lines touched, compiles shows whether or not the code compiled after the modernization, and fails PR merges shows if any of my sample set of pull requests (see this post) failed to merge against the modernized code.
|files||lines||compiles||fails PR merges|
A few available checks are not included in the table because they target a later C++ standard: modernize-make-unique and modernize-use-transparent-functors need C++14, and modernize-unary-static-assert needs C++17.
Some other checks (you can see them in the last rows of the table)
didn’t find anything to modernize. For a couple of them, this was
expected: modernize-avoid-bind works on uses of
whereas our C++03 code uses
modernize-make-shared works on
std::shared_ptr, also not present
in our legacy code. Replacing the Boost facilities with their
counterparts would make the checks useful; but I didn’t try the
conversion at this time.
The results of the remaining checks were, let’s say, less than satisfactory.
Trying modernize-replace-auto-ptr caused compilation errors, due
to a clang-tidy bug which I dutifully
filed. Long story
short: we have a class
Clone defined as
and clang-tidy replaced
std::unique_ptr in the
declaration of the constructor but not in the out-of-class definition.
The good news is that I wasn’t planning to rely on this check anyway.
Yes, it looks like a case of sour grapes. Lately, though,
has been causing deprecation warnings in the original code and I’ve
been thinking of conditionally replace it; you can read the details in
this GitHub issue.
Any solution to that would supersede the changes brought by this
check and make the bug moot.
Running modernize-pass-by-value resulted in filing another clang-tidy bug. The idea of the check is to replace idiomatic C++03 code like
with its modern C++ equivalent
which avoids copies entirely when the passed object is a temporary one. Unfortunately, as for the previous check, we had cases in which the declaration of the constructor was modified correctly but the definition wasn’t.
Next was modernize-use-using. It replaces
with the modern
using syntax; for instance,
How did it go? Let me put it this way: while filling the table at the beginning of the post, I briefly considered writing “Heck no” in the compiles column.
Due to another clang-tidy bug, sometimes template parameters in a typedef were replaced with their instantiation; for instance, the typedef in
would sometimes become
instead of the correct
I fixed a few declarations manually, but I threw the towel after spending ten minutes without finding the problem on a particularly hairy template instantiation. Also, for some reason some typedefs were matched by the check, the corresponding warnings was emitted, but the declaration was not converted. I was already bummed by that time, and didn’t try to figure out this one.
In any case, even if the check had worked, it wouldn’t have been the
end of the story for
using. The check only runs on existing
typedefs, but the real new feature is that
using gives you the
possibility to have the equivalent of template typedefs, missing in
C++03; that is, you can write things like
where in C++03 you have to resort to the clumsier
I’ll probably expand on the above in a future post, to show how we
could use the
using syntax in QuantLib (hint: the above is one such
case); but anyway, the transformation is a breaking change and thus
outside the scope of clang-tidy.
The modernize-loop-convert check was smarter than I thought. In absence of other information, it reuses for the loop variable the name of the original one; that is,
However, if the name of the container is plural, the check detects it and uses its singular for the name of the variable: for instance,
(This is not without glitches, though. Ever the trickster, I tried an
benches and it caused the loop variable to be named
benche. On the other hand, I also tried a case in which a
variable with the singular name was already defined, and the check
correctly chose a harmless
i for the name of the loop variable.)
In one case, the check even detected and reused a name that I had given to the element used in the iteration; that is,
Both of the above are nice touches, so kudos to the developers. The
only complaint I have is that the changes added a couple of warnings
to the compilation, and with my usual
-Wall -Werror flags this
caused the build to fail. The original code was
and the check was smart enough to figure out that the loop spanned the whole array, therefore it turned it into
nominalDataLength variable was no longer used,
hence the warning.
Finally, I don’t have a lot to say about modernize-return-braced-init-list, except that in one case the transformed code issued a warning. It turned the line
which caused a warning about narrowing the first of the returned constructor arguments.
So, what’s the verdict at the end of these three posts, apart from “most checks worked, some failed”?
In view of continuing the experiment (and possibly turn it in the future into a C++11 branch continuously integrating with master), I had hoped that the checks could be fully automated. This is not the case. True, some of the failures I described can be avoided by refactoring the original code; and if it came to that, others could be prevented by marking the corresponding lines so that clang-tidy doesn’t touch them. As a whole, though, the tool seems to me like the original Saturday Night Live crew: not ready for prime time. Let me stress it: this applies to our specific code base, and it might work perfectly for others. Try it out for yourselves.
However, clang-tidy remains a great tool to jump-start the process of modernization: the manual changes that need to be done after it runs are a lot less work than what you would have to do without the tool. And again, you might find that your code doesn’t need any.
A final note: as I write this, Ubuntu Bionic is out and includes version 6.0 of clang-tidy. Its release notes don’t seem to list changes that affect what I observed; but then again, I can’t exclude them. I’ll be checking it against the bugs I reported.
I’m not yet sure how this series will continue. I’ll probably try and consolidate in a single branch the changes I did so far; after which I might either add some tooling around it for continuous integration with master, or start monkeying around with other C++11 features. We’ll see what happens. In any case, I’ll keep you in the loop.
Follow me on Twitter if you want to be notified of new posts, or add me to your Google+ circles, or subscribe via RSS or email: the buttons for that are in the footer. Also, make sure to check my Training page.