7

I ran into a case recently where I had a const member function performing an operation and returning a result. For example,

class Foo { ...
    Foo add(Foo const & x) const;
}

But someone else was inadvertently calling it like it was updating the this object (ignoring the result):

Foo a = ...;
Foo b = ...;
a.add(b);

(This bug was actually introduced by an imperfect refactoring.)

Is there a way to make the last line above trigger an error or a warning? The next best thing would be a run-time catch, which is mostly addressed by the following template. However, it kills the return value optimization, as seen by the counter result.

template<typename T>
class MustTake {
    T & obj;
    bool took;
public:
    MustTake(T o) : obj(o), took(false) {}
    ~MustTake() { if (!took) throw "not taken"; }
    operator T&() { took = true; return obj;}
};

struct Counter {
    int n;
    Counter() : n(0) {}
    Counter(Counter const & c) : n(c.n+1) {}
    ~Counter() {}
};

Counter zero1() {
    return Counter();
}

MustTake<Counter> zero2() {
    return Counter();
}

int main() {
    Counter c1 = zero1();
    printf("%d\n",c1.n);    // prints 0
    Counter c2 = zero2();
    printf("%d\n",c2.n);    // prints 1
    zero1();    // result ignored
    zero2();    // throws
    return 0;
}

I suppose I can ameliorate the inefficiency by using a macro so that MustTake<> is debug only and a no-op for release.

I am looking for a compile-time solution. Failing that, I am looking for the best run-time solution.

xan
  • 7,511
  • 2
  • 32
  • 45
  • If you're using GCC, I think it has the `-Wunused-result` flag to enable a warning for that. – Xeo Jun 21 '11 at 23:52
  • Picking a better method name would be my suggestion. If I read `a.add(b)` I would immediately think mutator. How about `a.newListWithHead(b)` or something similar, depending on what's actually happening? Not as succinct as `add`, but adding isn't really what you are doing here. – clstrfsck Jun 22 '11 at 00:04
  • 4
    I like spong's idea of changing the name. For example, you could change it to `a + b` (if it makes sense to do so) and then everyone would know that "a + b" doesn't modify a, but they can easily write "a += b" to get the effect they wanted (I think). – David Grayson Jun 22 '11 at 00:10
  • @Sam and others, I'm using MSVC 2010 and GCC 3 (which I suppose will be Clang with Xcode 4). – xan Jun 22 '11 at 00:36
  • -Wunused-result tends to spew. A user is not going to see the warning about a.add(b) amidst the spew. – David Hammen Jun 22 '11 at 00:36
  • @spong, yes, the poor name was the source of the problem. The refactorer replaced Bar.plus() with Foo.add() without noticing Foo.add() returned its sum and Bar.plus() didn't. – xan Jun 22 '11 at 00:38
  • @David: But `-Wunused-result` exists, it works, it's documented, and *it's turned on by default.* – Dietrich Epp Jun 22 '11 at 00:38
  • -Wunused-result only works for functions with `warn_unused_result`. There are just too many system functions, along with `new` itself, where there is no warning about dropping an absolutely essential result on the floor. This is worse than a spew. It is an half-assed implementation. – David Hammen Jun 22 '11 at 01:05
  • gcc has `warn_unused_result`, msvc++ has `MustCheck` : from [other stackoverflow post](http://stackoverflow.com/questions/4226308/msvc-equivalent-of-attribute-warn-unused-result) – Tim Jun 22 '11 at 18:39

3 Answers3

8

This is what function attributes are for (documentation) in GCC and Clang, but it's not portable to e.g. MSVC.

class Foo { ...
    __attribute__((warn_unused_result))
    Foo add(Foo const & x) const;
}

The documentation says it's used on realloc for example, but it doesn't appear on any other standard functions on my system.

You may also be interested in using the Clang static analyzer, which tracks data flow across function calls and can give you better warnings.

Dietrich Epp
  • 205,541
  • 37
  • 345
  • 415
  • It's used on quite a few syscalls on my system (debian sid) - eg, `read()` and `write()`. In glibc it's `#define`d to `__wur` (this is an implementation detail of the C library, of course) – bdonlan Jun 22 '11 at 00:26
  • Huh, I didn't catch that with `grep` because it didn't find the definition of `__wur`. – Dietrich Epp Jun 22 '11 at 00:31
  • You can "make it portable" by wrapping it inside a macro that checks if the compiler is GCC or clang. For MSVC one may use the compiler flag `/W3` to enable warnings about unused variables. [See here](https://msdn.microsoft.com/en-us/library/c733d5h9.aspx). Although this enables it for *all* occurrences. – rwols Apr 13 '17 at 08:03
  • @rwols: Unfortunately unused variables != unused results. You can also `#define __attribute__()`. – Dietrich Epp Apr 13 '17 at 14:58
  • @DietrichEpp You're totally right, my bad. Aaaand I didn't even [see this answer](http://stackoverflow.com/a/6433557/990142). – rwols Apr 13 '17 at 15:23
7

For Microsoft VC++, there is the _Check_return_ annotation: http://msdn.microsoft.com/en-us/library/ms235402(v=VS.100).aspx

Nemanja Trifunovic
  • 24,346
  • 3
  • 50
  • 88
  • Is that different from MSVC++ `MustCheck` annotation? http://msdn.microsoft.com/en-us/library/ms182042(v=VS.90).aspx Why does microsoft have the two different sets of annotations? – Tim Jun 22 '11 at 23:10
0

If it is important that a return value not be ignored by the caller of a function, the template dont_ignore may be used as follows

BEFORE:

int add( int x, int y )
{
    return x + y;
}

AFTER:

dont_ignore<int> add( int x, int y )
{
    return x + y;
}

When the caller of the function does not use the return value, an exception is thrown. Definition of dont_ignore:

template<class T>
struct dont_ignore
{
    const T     v;
    bool        used;

    dont_ignore( const T& v )
        :  v( v ), used( false )
    {}

    ~dont_ignore()
    {
        if ( !used )
            throw std::runtime_error( "return value not used" );
    }

    operator T()
    {
        used = true;
        return v;
    }
};
zumalifeguard
  • 8,648
  • 5
  • 43
  • 56