Skip to content

3545: AVRO-4200: [C++] Encode and decode std::optional #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martin-augment wants to merge 1 commit into
base: main
Choose a base branch
from
Open

3545: AVRO-4200: [C++] Encode and decode std::optional #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions lang/c++/include/avro/Specific.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "array"
#include <algorithm>
#include <map>
#include <optional>
#include <string>
#include <vector>

Expand Down Expand Up @@ -165,6 +166,44 @@ struct codec_traits<double> {
}
};

/**
* codec_traits for Avro optional.
*/
template<typename T>
struct codec_traits<std::optional<T>> {
/**
* Encodes a given value.
*/
static void encode(Encoder &e, const std::optional<T> &b) {
if (b) {
e.encodeUnionIndex(1);
avro::encode(e, b.value());
} else {
e.encodeUnionIndex(0);
e.encodeNull();
}
}

/**
* Decodes into a given value.
*/
static void decode(Decoder &d, std::optional<T> &s) {
size_t n = d.decodeUnionIndex();
if (n >= 2) { throw avro::Exception("Union index too big"); }
Copy link
Copy Markdown

@augmentcode augmentcode bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header throws avro::Exception here, but Exception.hh isn’t included in this file; consider including it to avoid relying on transitive includes and ensure Specific.hh compiles standalone.

🤖 Was this useful? React with 👍 or 👎

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback:The Augment AI reviewer is correct that the import is needed to be able to build Specific.hh by itself. Currently it compiles only because some other import imports Exception.hh transitively

switch (n) {
case 0: {
d.decodeNull();
s = std::nullopt;
} break;
case 1: {
T t;
avro::decode(d, t);
s.emplace(t);
Copy link
Copy Markdown

@augmentcode augmentcode bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When decoding into std::optional<T>, this uses emplace(t) which requires a copy; consider constructing from a moved temporary to support move-only T and avoid unnecessary copies.

🤖 Was this useful? React with 👍 or 👎

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:good-to-have; category:bug; feedback:The Augment AI reviewer is correct that there is a implicit copy of memory here. It could be improved to use a std::move

Copy link
Copy Markdown

@cursor cursor bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Move Semantics for Optional Values

The decoded value t is being copied into the optional via s.emplace(t) instead of moved. This creates an unnecessary copy for types with expensive copy constructors. The array codec at line 296 uses std::move(t) in the same pattern, so this should use s.emplace(std::move(t)) or s = std::move(t) for consistency and efficiency.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:good-to-have; category:bug; feedback:The Bugbot AI reviewer is correct that there is a implicit copy of memory here. It could be improved to use a std::move

} break;
}
}
};

/**
* codec_traits for Avro string.
*/
Expand Down
15 changes: 15 additions & 0 deletions lang/c++/test/SpecificTests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

using std::array;
using std::map;
using std::optional;
using std::string;
using std::unique_ptr;
using std::vector;
Expand Down Expand Up @@ -127,6 +128,18 @@ void testDouble() {
BOOST_CHECK_CLOSE(b, n, 0.00000001);
}

void testNonEmptyOptional() {
optional<int64_t> n = -109;
optional<int64_t> b = encodeAndDecode(n);
BOOST_CHECK_EQUAL(b.value(), n.value());
}

void testEmptyOptional() {
optional<int64_t> n;
optional<int64_t> b = encodeAndDecode(n);
BOOST_CHECK(!b.has_value());
}

void testString() {
string n = "abc";
string b = encodeAndDecode(n);
Expand Down Expand Up @@ -191,6 +204,8 @@ init_unit_test_suite(int /*argc*/, char * /*argv*/[]) {
ts->add(BOOST_TEST_CASE(avro::specific::testLong));
ts->add(BOOST_TEST_CASE(avro::specific::testFloat));
ts->add(BOOST_TEST_CASE(avro::specific::testDouble));
ts->add(BOOST_TEST_CASE(avro::specific::testNonEmptyOptional));
ts->add(BOOST_TEST_CASE(avro::specific::testEmptyOptional));
ts->add(BOOST_TEST_CASE(avro::specific::testString));
ts->add(BOOST_TEST_CASE(avro::specific::testBytes));
ts->add(BOOST_TEST_CASE(avro::specific::testFixed));
Expand Down
Loading