I was experimenting with a simple code for calculating cosine similarity:

#include <iostream>
#include <numeric>
#include <array>
#include <cmath>

float safe_divide(const float& a, const float& b) { return b < 1e-8f && b > -1e-8f ? 0.f : a / b; }

template< size_t N >
float cosine_similarity( std::array<float, N> a, std::array<float, N> b )
{
    const float&& a2 = std::move( std::inner_product( a.begin(), a.end(), a.begin(), 0.f ) );
    const float&& b2 = std::move( std::inner_product( b.begin(), b.end(), b.begin(), 0.f ) );
    const float&& dot_product = std::move( std::inner_product( a.begin(), a.end(), b.begin(), 0.f ) );

    return safe_divide( dot_product, ( std::sqrt(a2) * std::sqrt(b2) ) );
}

int main(){
    std::array<float, 5> a{1,1,1,1,1}, b{-1,1,-1,1,-1};
    std::cout<<cosine_similarity(a,b);  
}

On x86-64 Clang 12.0.1 (and other versions), it compiles and gives the right answer.
However on every version of GCC that I've tested it compiles, but gives the wrong answer (or no answer).

It raises a few questions:

1. Is my use of std::move even valid?
2. Why does only Clang seem to work with this and no other compiler?
3. What does the standard say?

Here's a link to the experiment: https://godbolt.org/z/KWbMYorrc

CodePudding user response：

What is happening is:

• std::inner_product( a.begin(), a.end(), a.begin(), 0.f ) returns a temporary, whose lifetime normally ends at the end of the statement
• when you assign a temporary directly to a reference, there is a special rule that extends the life of the temporary
• however, the problem with: std::move( std::inner_product( b.begin(), b.end(), b.begin(), 0.f ) ); is that the temporary is no longer assigned directly to a reference. Instead it is passed to a function (std::move) and its lifetime ends at the end of the statement.
• std::move returns the same reference, but the compiler doesn't intrinsically know this. std::move is just a function. So, it doesn't extend the lifetime of the underlying temporary.

That it appears to work with Clang is just a fluke. What you have here is a program exhibiting undefined behaviour.

See for example this code (godbolt: https://godbolt.org/z/nPGxMnrzf) which mirrors your example to some extent, but includes output to show when objects are destroyed:

#include <iostream>

class Foo {
    public:
    Foo() { std::cout << "Foo was created\n"; }
    ~Foo() { std::cout << "Foo was destroyed\n"; }
};

Foo getAFoo() {
    return Foo();
}

Foo &&doBadThings() {
    Foo &&a = std::move(getAFoo());
    Foo &&b = std::move(getAFoo());
    std::cout << "If Foo objects have been destroyed, a and b are dangling refs...\n";
    return std::move(a);
}

int main() {
    doBadThings();
}

Output is:

Foo was created
Foo was destroyed
Foo was created
Foo was destroyed
If Foo objects have been destroyed, a and b are dangling refs...

In this case Clang and Gcc both produce the same output, but it's enough to demonstrate the problem.

CodePudding user response：

First the question you did not ask:

4. Does it make sense to use move semantics in this code?

No. Moving a float is actually exactly the same as copying a float. You could even consider to pass the parameters by value, because passing them by reference won't significantly speed things up (though, don't believe me, measure).

#include <iostream>
#include <numeric>
#include <array>
#include <cmath>

float safe_divide(float a, float b) { return b < 1e-8f && b > -1e-8f ? 0.f : a / b; }

template< size_t N >
float cosine_similarity( std::array<float, N> a, std::array<float, N> b )
{
    return safe_divide( std::inner_product( a.begin(), a.end(), b.begin(), 0.f ), 
                        std::sqrt(std::inner_product( a.begin(), a.end(), a.begin(), 0.f )) 
                      * std::sqrt(std::inner_product( b.begin(), b.end(), b.begin(), 0.f )) );
}

int main(){
    std::array<float, 5> a{1,1,1,1,1}, b{-1,1,-1,1,-1};
    std::cout<<cosine_similarity(a,b);  
}

In this code, the values returned from the calls to inner_product are already temporaries. There is no need to use std::move to cast them to r-value references.

1. Is my use of std::move even valid?

Actually it is not directly the call to std::move that is the problem. The issue is that you keep references to temporaries whose lifetime ends at the end of the line. Here

const float&& a2 = std::move( std::inner_product( a.begin(), a.end(), a.begin(), 0.f ) );
const float&& b2 = std::move( std::inner_product( b.begin(), b.end(), b.begin(), 0.f ) );
const float&& dot_product = std::move( std::inner_product( a.begin(), a.end(), b.begin(), 0.f ) );

Those references are dangling. The temporaries cease to exist at the end of the expressions.

3. What does the standard say?

Reading from a dangling reference is undefined behavior.

2. Why does only Clang seem to work with this and no other compiler?

Because undefined behavior is undefined.

PS: Deliberately I tried to use simple language, thats the language I can understand and speak ;). The details of value categories and extending lifetime of temporaries by binding them to references are more involved than this answer might suggest.