On Mon, Feb 12, 2024 at 7:04 AM Patrice Roy via SG14 <sg14_at_[hidden]>
wrote:

>
> * P3132 Accept attributes with user-defined prefixes
> * P3134 Attribute [[asserts_rvo]]
>
> P3132 aims to make it easier to do tooling through user-defined attributes
> and has a number of additional upsides.
> P3134 aims to provide guarantees that a function's implementation can make
> it possible for appropriately written calls to that function to lead to the
> return value optimization
>
> [...] If you would be so kind as to (a) take a look at them, and ideally
> (b) provide suggestions / constructive criticism on this list or at the
> SG14 meeting this week, I would integrate this feedback into the official
> papers. In particular, I get the impression that the audience should be
> LEWG but I'd be open to knowing what the groups thinks.
>

Both papers propose core-language features with no library impact, so they
should be EWG (in fact, EWGI).

Here's my feedback on P3134:

You propose an attribute to basically "mandate NRVO."
This is superficially similar to an idea I've had on the mental back burner
for a long time: an attribute to force a designated variable into the
return slot.
- Re: [std-proposals] Return Value Optimisation whenever you need it
(guaranteed elision)
<https://lists.isocpp.org/std-proposals/2023/08/7422.php> (August 2023)
- Re: [std-proposals] Copy-construct, move-construct, and PR-construct
<https://lists.isocpp.org/std-proposals/2023/08/7537.php> (August 2023)
My idea (which I'd love to see in practice, if anyone with Clang-hacking
skills wants to prototype it!) is to mark a `return` statement as "NRVO
guaranteed/mandated," so that (1) the programmer can be sure no extra move
or copy happens invisibly there, and (2) the programmer can even return a
non-copyable/non-movable type!

    // https://godbolt.org/z/E9TcraM4c
    static_assert(__has_cpp_attribute(fantasy::nrvo)); // so that we can
rely on its guarantees
    struct S { S(int); S(const S&); ~S(); void f(); };

    S annotated(bool cond) {
      S x = S(42);
      x.f();
      if (cond) {
        *[[fantasy::nrvo]]* return x;
      } else {
        return S(43);
      }
    }

Here the compiler would be required to either (1) return `x` without an
extra call to `S(const S&)`, or (2) give an error diagnostic on that line.
Some compilers would be "sufficiently smart" to basically hoist the `if` up
above the construction of `x`, and make `x`'s address depend on `cond`.
Insufficiently smart compilers (a category which includes Clang and GCC as
of this writing) would simply report the error.

An attribute is also mentioned (only to dismiss, with a rationale I think
is wrong these days) in Anton Zhilin's P2025R2 "Guaranteed copy elision for
return variables"
<https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2025r2.html#explicit-mark-attribute>
.

GCC 3.1 through 3.4 actually had non-standard syntax for creating an NRVO
variable <https://gcc.gnu.org/onlinedocs/gcc-2.95.3/gcc_5.html#SEC106>.
I've never used it in anger, and it looks like such old GCCs aren't even
available on Godbolt anymore (I could've sworn 2.95 was available for a
while! but I don't see it now). But the syntax AIUI looked like this (broken
Godbolt <https://godbolt.org/z/s7eMfTMoK>):

    struct S { S(int); S(const S&); ~S(); void f(); };

    S unannotated(bool cond) {
      S x = S(42);
      x.f();
      if (cond) {
        return x;
      } else {
        return S(43);
      }
    }

    S annotated(bool cond) return x(42); {
      x.f();
      if (cond) {
        return x;
      } else {
        return S(43);
      }
    }

The latter constructs `x` directly into the return slot. Then, if `!cond`,
we'll destroy `x` before constructing `S(43)` back into the return slot.
(This sounds like a recipe for use-after-destroy bugs to me! But like I
said, I never used the feature.)

Now, what you propose in P3134 is significantly different from either of
these approaches. You propose to annotate the *function header*, rather
than the specific variable or specific return statement that you're
interested in. This already strikes me as a bad idea, because it isn't
expressive enough to handle (either version of) our `annotated` function
above. How's the compiler supposed to discover which of the two return
statements you're talking about? In my [[nrvo]] idea, the correct statement
is annotated directly. In GCC's prior art, the correct statement returns
the NRVO variable. But how does your proposal indicate that we care about
`return x` and not `return S(43)`?

(Notice that we cannot place *both* `x` and `S(43)` in the return slot. I
mean, GCC does, but it cheats by destroying `x` early. And more complicated
examples exist where even that kind of cheating is impossible.)

Furthermore, I don't understand why you think it's important to annotate
the function *declaration*, when NRVO is a feature solely of the function
*body*. Why should the user of the function care whether the function is
annotated with [[asserts_rvo]]? That has nothing to do with whether the
function *as a whole* is performant. For example,
    [[asserts_rvo]] S f() {
        S a = S(42);
        S b = std::move(a);
        return b;
    }
would be totally fine by P3134's standards, but it clearly does one more
move-and-destroy than it "should" do.
Compare the situation with C++20 Coroutines. We write
    std::generator<int> primes();
and not e.g.
    async std::generator<int> primes();
because from the user's point of view it *simply doesn't matter* how the
`primes` function is implemented internally. All they care about is that
they get out something of the proper type. NRVO works the same way:
invisibly to the user.

Also, consider that on most platforms NRVO is incompatible with trivial
types.
    struct Simple { int i; };
    Simple g() {
      Simple x = {42};
      return x; // cannot be NRVO'ed!
    }
What would your attribute do here? Suppose I put the attribute on the
function declaration in my header file:
    [[asserts_rvo]] Simple g();
Is that basically a compile-time assertion that `Simple` isn't
trivial-for-return on my platform's ABI? That's... thought-provoking... but
my initial reaction is that we don't want that kind of thing.

IMO the most attractive part of P3134 is the one part you explicitly say
you're not pursuing:
    [[asserts_rvo]] SomeClass someObj = ExternalFoo();
I've frequently needed something like this in production. The problem is
that you'll be making a copy of some getter's result, and you want to make
sure you're not unnecessarily copying but also not unintentionally dangling
a reference. For example: If `vector<string> hostnames()`, then
    vector<string> names = server.hostnames();
is perfectly efficient — but if the getter changes to `const
vector<string>& hostnames()`, then suddenly it'll lose efficiency! So it's
kind of dangerous to write. We could write
    const vector<string>& names = server.hostnames();
which is always efficient, but it relies on reference lifetime extension,
which is hard to teach, so I'd rather avoid it when possible. You can get
the best of both worlds with
    decltype(auto) names = server.hostnames();
but then you lose the name of the type. C++20 finally allows
    std::same_as<vector<string>> decltype(auto) names = server.hostnames();
whose only downside is verbosity; and in fact pre-C++20 we can (and I
sometimes do) write a two-liner:
    static_assert(std::is_same_v<vector<string>,
decltype(server.hostnames())>, "no const&");
    vector<string> names = server.hostnames();
But wouldn't it be nice to write something like
    [[rvo]] vector<string> names = server.hostnames();
and have it be readable *and* efficient *and*
proof-against-future-changes-to-the-getter all at once?
(However, I recognize that the party line here is "Just use reference
lifetime extension; you're making a problem where none exists.")

TLDR I think the paper's biggest flaws are:
- it doesn't explain why the *user* of a function should care about seeing
this attribute on its signature at all
- it doesn't explain how the compiler is supposed to treat this attribute
when there are multiple `return` statements in a function

Oh, and one of the example snippets mentions "P1155 proposes to move" —
P1155 has been the law of the land since C++20, and in fact was superseded
by P2266 in C++23. You should at least avoid anachronistic comments in
examples, and probably re-audit that example to see if it still makes any
sense in the year 2024. Remember, "I'm stuck using an old compiler" is a
realistic *problem*, but it isn't a valid *argument* for why new compilers
should add features (like a new attribute) — your old compiler won't
magically acquire those new features!

my $.02,
–Arthur