Search code examples
c++stlstatic-analysis

Rationale for explicit conversion operator on functor


While I was on a short break, my workplace switched to using a static code analyzer. They ran it over the project I am working on and one particular problem flagged by the analyzer goes like this (simplified example):

struct calcSomething
{
    int result;

    calcSomething() : result(0) {}

    void operator()(const int v) { /*does something*/ }
    operator int() const { return result; }
};

void foo()
{
    std::vector<int> myvector(10);
    // exercise for reader: stick some values in `myvector`

    int result = std::for_each(myvector.begin(), myvector.end(), calcSomething());
}

The analyzer flags the following issues:

warning: CodeChecker: 'operator int' must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
    operator int() const { return result; }

The suggested fix to the functor reads:

struct calcSomething
{
    ... 
    explicit operator int() const { return result; }
};

But if I fix my functor as suggested, the static analyzer quickly flags the following issue:

warning: CodeChecker: no viable conversion from '(anonymous namespace)::calcSomething' to 'int' [clang-diagnostic-error]

I now need to add the explicit cast:

void foo()
{
    ...
    int total = static_cast<int>(std::for_each(myvector.begin(), myvector.end(), calcSomething()));
}

The above example is a mere simplification of the real problem which would otherwise just add filler and no substance.

I have seen plenty of examples of functors like the one I describe here in text books and programming reference web pages. I have never considered these unsafe. I have never seen anyone flag these as unsafe.

So does the code analyzer have a point? Or is it a little overzealous to make my functor's conversion operator explicit and as a result make me add the static cast?

Purely from an aesthetic point, I feel that a simple problem with an elegant solution now accrues a lot of ugly syntactic padding. But perhaps that is the price we pay for writing safe(r) code.

side note: TIL that explicit applies not only to ctors

Edit

It seems some people are unable to read beyond the example code I provided (pretty textbook stuff) and still suggest other algorithms/idioms, completely failing to see that the actual question is about conversion operators on functors whose sole purpose is to calculate and return an algorithm's result.

If the question was about how to improve on an adding algorithm, then the title would have said so.

So I decided to hide any implementation details in this edit to make it easier for these people.

Sorry that some of the comments below now no longer make any sense, but the record got stuck so I had to move the needle a bit in order to move things forward (hopefully).


Solution

  • I would do away with any conversion operators altogether. What's wrong with:

    int result = std::for_each(...).get();
    

    where get() does the same as your current operator int.

    You know that the result of for_each is not an integer, it's your function object. Why, why would you want to avoid making the conversion from a function to a value explicit? It is, by all means, a questionable idea. Sure, you can still do it, but you want clean warning-free code right? Well, clean, warning free code, in my book, should not auto-convert functions to integers. I agree static_cast is almost equally ugly, that's why I am suggesting a named function