Leaving C++03: automation, part 3

Greetings.

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.

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 std::bind, whereas our C++03 code uses boost::bind, and modernize-make-shared works on std::shared_ptr, also not present in our legacy code. Replacing the Boost facilities with their std 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

    template <class T>
    class Clone {
      public:
        Clone(std::auto_ptr<T>);
        // other constructors, methods, etc.
    };

    template <class T>
    inline Clone<T>::Clone(std::auto_ptr<T> p)
    : ptr_(p) {}

and clang-tidy replaced std::auto_ptr with std::unique_ptr in the declaration of the constructor but not in the out-of-class definition. Hilarity ensued.

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, auto_ptr 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

    class Foo {
      public:
        Foo(const Bar& b) : b_(b) {}
      private:
        Bar b_;
    };

with its modern C++ equivalent

    class Foo {
      public:
        Foo(Bar b) : b_(std::move(b)) {}
      private:
        Bar b_;
    };

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 typedef statements with the modern using syntax; for instance,

    typedef Sample<Real> sample_type;

becomes

    using sample_type = Sample<Real>;

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

    template <class Container>
    class Foo {
      public:
        typedef typename Container::const_iterator const_iterator;
    };

would sometimes become

    template <class Container>
    class Foo {
      public:
        using const_iterator = typename map<int, double, less<int>,
                allocator<pair<const int, double> > >::const_iterator;
    };

instead of the correct

    template <class Container>
    class Foo {
      public:
        using const_iterator = typename Container::const_iterator;
    };

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

    struct Discount {
        template <class T>
        using curve = InterpolatedDiscountCurve<T>;
    };

    class C : public Discount::curve<Linear> {};

where in C++03 you have to resort to the clumsier

    struct Discount {
        template <class T>
        struct curve {
            typedef InterpolatedDiscountCurve<T> type;
        };
    };

    class C : public Discount::typename curve<Linear>::type {};

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,

    for (Size i = 0; i < basket.size(); ++i)
        basket[i]->setPricingEngine(swaptionEngine);

becomes

    for (auto & i : basket)
        i->setPricingEngine(swaptionEngine);

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,

    for (Size i=0; i < means.size(); ++i)
        std::cout << means[i] << "\n";

becomes

    for (double mean : means)
        std::cout << mean << "\n";

(This is not without glitches, though. Ever the trickster, I tried an array named 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,

    for (Size i=0; i<callabilityTimes_.size(); i++) {
        Time exerciseTime = callabilityTimes_[i];
        // use exerciseTime
    }

became

    for (double exerciseTime : callabilityTimes_) {
        // use exerciseTime
    }

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

    Datum nominalData[] = {
        // data
    };
    const Size nominalDataLength = 29; // the size of the array

    for (Size i = 0; i < nominalDataLength; i++) {
        // use nominalData[i]
    }

and the check was smart enough to figure out that the loop spanned the whole array, therefore it turned it into

    Datum nominalData[] = {
        // data
    };
    const Size nominalDataLength = 29; // the size of the array

    for (auto & i : nominalData) {
        // i
    }

Unfortunately, the 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

    return Date((1 + dayOfWeek + skip*7) - first, m, y);

into

    return {(1 + dayOfWeek + skip*7) - first, m, y};

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 or LinkedIn if you want to be notified of new posts, or subscribe via RSS: the buttons for that are in the footer. Also, I’m available for training, both online and (when possible) on-site: visit my Training page for more information.