3

Consider the following mwe, implementing a simple class that overloads the multiply operator:

#include <iostream>

using namespace std;

class A {
public:
  double a;
  
  A &operator*(double b)
  {
        a *= b;
        return *this;
  }
    
  friend A &operator*(double b, A &m) { return m * b; } // (*)
};

int main()
{
    
    A a;
    a.a = 5.0;
    a * 3.0;
    std::cout << a.a << std::endl;
    3.0 * a;
    std::cout << a.a << std::endl;
    return 0;
}

It compiles and runs just fine, printing the expected output. However cppcheck gives the following warning.

tmp.cpp:15:48: error: Reference to temporary returned. [returnTempReference]
friend A &operator*(double b, A &m) { return m * b; }

When rewriting (*) as

friend A &operator*(double b, A &m) { m * b; return m; }

The error from cppcheck disappears.

So, is this an actual problem and I'm overseeing something, or is it a false positive by cppcheck?

Some context: In my code, class A is actually a Matrix class, for which I don't want to create a new object upon multiplying with a constant, thus returning the reference.

SimonH
  • 1,385
  • 15
  • 35

3 Answers3

6

Your code does not return a reference to a temporary. One is inclided to think that, because what you implemented as operator* is actually an operator*=. If you did implment operator* which does not modify the current A but returns a new one, then returning a reference would be problematic.

I dont have the tool available, but I would expect it to not complain about:

#include <iostream>

using namespace std;

class A {
public:
  double a;
  
  A &operator*=(double b)
  {
        a *= b;
        return *this;
  }
    
  friend A &operator*=(double b, A &m) { return m *= b; } // (*)
};

int main()
{
    
    A a;
    a.a = 5.0;
    a *= 3.0;
    std::cout << a.a << std::endl;
    3.0 *= a;
    std::cout << a.a << std::endl;
    return 0;
}

Which really does the same as your code, but with the expected semantics of *=.

As songyuanyao mentions in a comment, it might be that the tool simply expects m*b to be a rvalue-expression, as it usually is, without actually checking the implementation of your *.

463035818_is_not_an_ai
  • 109,796
  • 11
  • 89
  • 185
  • Thanks for the answer. I purposely did not implement `operator*=` because I want to chain multiple multiplications in one line, for which I need `operator*` I guess. But for this simplified scenario your adjustment using `operator*=` works fine. – SimonH Aug 29 '23 at 11:50
  • 2
    @SimhonH thats a misunderstanding you can chain `*=` just like you can `*`. – 463035818_is_not_an_ai Aug 29 '23 at 11:53
  • How would chaining `*=` look like? When changing `a *= 3.0;` into `a *= 3.0 * a;` using your rewritten code, the compiler yields an error, complaining that `operator*` is not defined for arguments of type double and A. – SimonH Aug 29 '23 at 11:58
  • Nvm, I figured it out. I honestly didn't know that one can chain `*=` and I'm definitely not a fan of the look of it. – SimonH Aug 29 '23 at 12:07
  • You'd need to override the precedence: `(a *= 3.0) *= a;` or use a sequence of statements: `a *= 3.0; a *= a;`. – Toby Speight Aug 29 '23 at 12:20
  • 2
    @SimonH the thing is that `a * b * c` is expected to not modify any of its operands by most coders used to idiomatic c++. Your `operator*` breaks the principle of least surprise. Others using your `*` will very likely write broken code and then wonder why. `operator*=` on the other hand is expected to do exactly what you implemented – 463035818_is_not_an_ai Aug 29 '23 at 16:02
  • 1
    @SiinmH btw if you interested in the issue of avoiding the temproies in something like `a = a * a + 3.0;` then you might be interested in expression templates. Thats how algebra libraries approach this. In a nutshell, it is about returning a proxy object from `operator*` and delay the actual computation until the result is actually needed – 463035818_is_not_an_ai Aug 29 '23 at 17:45
  • @463035818_is_not_an_ai Wow nice, expression templates actually exactly look like what I wanted when I wrote my matrix code. Thanks for the hint! – SimonH Aug 30 '23 at 07:00
3

It is a false positive on cppcheck side, but also your code is highly confusing to any readers (as shown in comments) because you don't obey basic rules and idioms for operator overloading. It may or not be related to cppcheck mistake.

Proper implementation of binary operator * should return a copy:

class A {
public:
  double a;
  // no need for friend declarations, your data member is public
};

A operator*(A a, double b)
{
    A.a *= b;
    return a;
}

A operator* (double b, A a)
{
    return a * b;
}
Yksisarvinen
  • 18,008
  • 2
  • 24
  • 52
  • Hm yeah I see. But when chaining multiple multiplications together, each creating a new really big temporary matrix, this idiomatic approach loses its attraction imo. I guess I should've put this information in the question, too. – SimonH Aug 29 '23 at 11:53
  • 3
    @SimonH Then implement `*=` operator which modifies it's argument directly (see [the other answer](https://stackoverflow.com/a/76999896/7976805)). Although I'd be strongly opposed to implementing `operator *= (double, A)`, that's very unidiomatic. – Yksisarvinen Aug 29 '23 at 11:57
2

It looks like a misleading warning. I wouldn't call it a "false positive", because this sort of code does warrant a warning, but a different warning.

Specifically, the member operator* should be const, and return a temporary by value. This overload of operator* is clearly supposed to be a product (in mathematical sense) considering it commutes.

I understand the idea of multiplying with a constant, and not creating a temporary. You can express that as Matrix operator*(Matrix&& lhs, float rhs). The rvalue reference makes explicit that lhs is altered.

MSalters
  • 173,980
  • 10
  • 155
  • 350