NRVO not eliding copy for `std::string` with early exit

[firewave]

#include <string>

std::string f(int maxsize)
{
    std::string ret;
    for (int i = 0; ; ++i)
    {
        if (i > maxsize)
            return "";
        ret += ".";
    }
    return ret;
}

<source>:12:12: warning: not eliding copy on return [-Wnrvo]
   12 |     return ret;
      |            ^

https://godbolt.org/z/fPfEGMh7E

Clearing ret and returning it addresses the warning but feels a bit awkward.

This also occurs without the loop but such a case can easily be mitigated without uglifying the code.

[llvmbot]

@llvm/issue-subscribers-c-1

Author: Oliver Stöneberg (firewave)

```cpp #include <string>

std::string f(int maxsize)
{
std::string ret;
for (int i = 0; ; ++i)
{
if (i > maxsize)
return "";
ret += ".";
}
return ret;
}

<source>:12:12: warning: not eliding copy on return [-Wnrvo]
12 | return ret;
| ^

https://godbolt.org/z/fPfEGMh7E

Clearing `ret` and returning it addresses the warning but feels a bit awkward.

This also occurs without the loop but such a case can easily be mitigated without uglifying the code.
</details>

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Oliver Stöneberg (firewave)

```cpp #include <string>

std::string f(int maxsize)
{
std::string ret;
for (int i = 0; ; ++i)
{
if (i > maxsize)
return "";
ret += ".";
}
return ret;
}

<source>:12:12: warning: not eliding copy on return [-Wnrvo]
12 | return ret;
| ^

https://godbolt.org/z/fPfEGMh7E

Clearing `ret` and returning it addresses the warning but feels a bit awkward.

This also occurs without the loop but such a case can easily be mitigated without uglifying the code.
</details>

[firewave]

Possibly the same issue:

#include <vector>

std::vector<int> f()
{
    std::vector<int> v;
    if (!v.empty())
        return v;
    return {};
}

<source>:7:16: warning: not eliding copy on return [-Wnrvo]
    7 |         return v;
      |                ^

https://godbolt.org/z/hv368qa1G

This is a massive simplification from the original code where it is not feasible to create an initial empty container so it can be returned in a conditional block.

[zygoloid]

It would not be correct to perform NRVO in this situation. The local variable ret (or v in the second example) must be destroyed after the returned object is constructed according to the language rules, so we can't treat ret (v) as a name for the return object. (It's somewhat pathological, but the construction of the new return value could observe whether the old one has been destroyed or not, for example through a global variable, so we can't reorder the construction and destruction.)

Also, producing a -Wnrvo warning here seems appropriate, especially since the code appears to intend to perform NRVO, and there'd be a fairly simple modification to the code that would allow NRVO to occur. (Clear ret / v and return it.)

What would you like Clang to do differently here?

[firewave]

Thanks for the thoroughly explanation.

Also, producing a -Wnrvo warning here seems appropriate, especially since the code appears to intend to perform NRVO, and there'd be a fairly simple modification to the code that would allow NRVO to occur. (Clear ret / v and return it.)

As I pointed out the simplified cases here are easy to fix but the actual code which produces this would become quite messy (and more bugprone and harder to understand) if you would base it around a single variable as the only way to return.

[firewave]

I have been reviewing more warnings and now I understand this better I can solve a lot with additional scoping. There's still some rather awkward cases. I will show some examples after I am done with the low-hanging fruits.

[firewave]

FYI fixes to some of the warnings we encountered: danmar/cppcheck#7883. As mentioned before they can mostly be addressed by introducing additional scopes which makes the code looks a bit strange but as allows to reduces the lifetimes of some objects it might even improve the code beyond NRVO.

Still need to look through the remaining ones.