CPlusPlusGenerator: Avoid unnecessary copy by using a const ref to the input vector

[maddouri]

The current version of bebopc generates, among others, decoding methods that take in an input byte vector by copy (i.e. std::vector<uint8_t>), e.g. the Playground's "Hello World" sample (as of 2024/10/22 22:30 UTC):

static Hello decode(std::vector<uint8_t> sourceBuffer);
static size_t decodeInto(std::vector<uint8_t> sourceBuffer, Hello& target);

That can be quite inefficient depending on the input vector's size, compiler optimizations and/or the caller's use of e.g. std::move().
In addition, given that those calls boil down to calling the overloads that take const uint8_t* sourceBuffer anyway, it seems that there isn't any actual need for copying the input vector and manipulating a local mutable version of it.

Therefore, I'd like to submit this patch to the C++ code generator which changes the input vector copy (i.e. std::vector<uint8_t>) into a const reference (i.e. const std::vector<uint8_t>&).
When applied to the "Hello world" example of the playground, the patched bebopc would generate methods that have the following prototypes:

static Hello decode(const std::vector<uint8_t>& sourceBuffer);
static size_t decodeInto(const std::vector<uint8_t>& sourceBuffer, Hello& target);

Thank you in advance for considering this PR!

[andrewmd5]

Thank you for the PR. Just a question - the compiler generates:

static size_t decodeInto(const uint8_t* sourceBuffer, size_t sourceBufferSize, Hello& target) {
    ::bebop::Reader reader{sourceBuffer, sourceBufferSize};
    return Hello::decodeInto(reader, target);
  }

This shouldn't incur any copy overhead and gives you better flexibility, does this not align with your use case?

[maddouri]

This shouldn't incur any copy overhead and gives you better flexibility, does this not align with your use case?

You're right. However, IMO, there seems to be some sort of "asymmetry" in the generated code's interface: i.e. I find it "surprising" to use a reference in the "encode" directions, e.g.

static size_t encodeInto(const Hello& message, std::vector<uint8_t>& targetBuffer);

... and then use a copy in the "decode" direction, e.g.

static size_t decodeInto(std::vector<uint8_t> sourceBuffer, Hello& target);

A careful reading of the generated code might prevent some developer from making the mistake of calling the overload that results in a copy, but I would still argue that the generated code should be be as consistent and "unsurprising" as possible.

As a sidenote, and to keep up with that "symmetry" theme, one could also wonder about the lack of "encoding" methods that would take uint8_t* destinationBuffer, size_t destinationBufferSize... but that could be a topic for another PR 😉