Skip to content

Optimize C++ generated code for structures with many conditional fields #235

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

Closed
AaronWebster wants to merge 2 commits into from
Closed

Optimize C++ generated code for structures with many conditional fields #235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a14004
Port optimized conditionals v2
AaronWebster Dec 2, 2025
8ea51fb
feat(cpp): Optimize generated C++ accessors and control flow
AaronWebster Dec 3, 2025
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
12 changes: 12 additions & 0 deletions compiler/back_end/cpp/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,17 @@ emboss_cc_test(
],
)

emboss_cc_test(
name = "many_conditionals_benchmark",
srcs = [
"testcode/many_conditionals_benchmark.cc",
],
deps = [
"//testdata:many_conditionals_emboss",
"@com_google_googletest//:gtest_main",
],
)

# New golden test infrastructure
py_library(
name = "one_golden_test_lib",
Expand Down Expand Up @@ -535,6 +546,7 @@ cpp_golden_test(
cpp_golden_test(
name = "no_enum_traits_golden_test",
emb_file = "//testdata:no_enum_traits.emb",
extra_args = ["--no-cc-enum-traits"],
golden_file = "//testdata/golden_cpp:no_enum_traits.emb.h",
)

Expand Down
5 changes: 3 additions & 2 deletions compiler/back_end/cpp/build_defs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,15 @@ def emboss_cc_test(name, copts = None, no_w_sign_compare = False, **kwargs):
**kwargs
)

def cpp_golden_test(name, emb_file, golden_file, import_dirs = []):
def cpp_golden_test(name, emb_file, golden_file, import_dirs = [], extra_args = []):
"""Defines a C++ golden file test.

Args:
name: The name of the test.
emb_file: The .emb file to test.
golden_file: The golden .h file.
import_dirs: A list of import directories.
extra_args: A list of extra arguments to pass to the compiler.
"""
py_test(
name = name,
Expand All @@ -65,7 +66,7 @@ def cpp_golden_test(name, emb_file, golden_file, import_dirs = []):
"$(location :emboss_codegen_cpp)",
"$(location %s)" % emb_file,
"$(location %s)" % golden_file,
] + ["--import-dir=" + d for d in import_dirs],
] + ["--import-dir=" + d for d in import_dirs] + extra_args,
data = [
"//compiler/front_end:emboss_front_end",
":emboss_codegen_cpp",
Expand Down
86 changes: 45 additions & 41 deletions compiler/back_end/cpp/generated_code_templates
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,16 @@ class Generic${name}View final {

${enum_usings}

bool Ok() const {
[[nodiscard]] bool Ok() const {
Copy link
Collaborator

@EricRahm EricRahm Dec 3, 2025

Choose a reason for hiding this comment

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

I appreciate adding these annotations, but maybe we should pull them out into a separate issue and PR to discuss them in isolation. I believe it's a C++ 17 feature and we may need to hash out some details around supporting (or not) C++14.

if (!IsComplete()) return false;
${parameter_ok_checks}
${ok_subexpressions}
${field_ok_checks}
${requires_check}
return true;
}
Storage BackingStorage() const { return backing_; }
bool IsComplete() const {
[[nodiscard]] Storage BackingStorage() const { return backing_; }
[[nodiscard]] bool IsComplete() const {
return backing_.Ok() && IntrinsicSizeIn${units}().Ok() &&
backing_.SizeIn${units}() >=
static_cast</**/ ::std::size_t>(
Expand All @@ -141,12 +142,12 @@ ${requires_check}
${size_method}

template <typename OtherStorage>
bool Equals(
[[nodiscard]] bool Equals(
Generic${name}View<OtherStorage> emboss_reserved_local_other) const {
${equals_method_body} return true;
}
template <typename OtherStorage>
bool UncheckedEquals(
[[nodiscard]] bool UncheckedEquals(
Generic${name}View<OtherStorage> emboss_reserved_local_other) const {
${unchecked_equals_method_body} return true;
}
Expand All @@ -171,7 +172,7 @@ ${size_method}
emboss_reserved_local_other.IntrinsicSizeIn${units}().Read());
}
template <typename OtherStorage>
bool TryToCopyFrom(
[[nodiscard]] bool TryToCopyFrom(
Generic${name}View<OtherStorage> emboss_reserved_local_other) const {
return emboss_reserved_local_other.Ok() && backing_.TryToCopyFrom(
emboss_reserved_local_other.BackingStorage(),
Expand All @@ -180,7 +181,7 @@ ${size_method}

${text_stream_methods}

static constexpr bool IsAggregate() { return true; }
[[nodiscard]] static constexpr bool IsAggregate() { return true; }

${field_method_declarations}

Expand Down Expand Up @@ -215,7 +216,7 @@ struct EmbossReservedInternalIsGeneric${name}View<
};

template <typename T>
inline Generic${name}View<
[[nodiscard]] inline Generic${name}View<
/**/ ::emboss::support::ContiguousBuffer<
typename ::std::remove_reference<
decltype(*::std::declval<T>()->data())>::type,
Expand All @@ -230,7 +231,7 @@ Make${name}View(${constructor_parameters} T &&emboss_reserved_local_arg) {
}

template <typename T>
inline Generic${name}View</**/ ::emboss::support::ContiguousBuffer<T, 1, 0>>
[[nodiscard]] inline Generic${name}View</**/ ::emboss::support::ContiguousBuffer<T, 1, 0>>
Make${name}View(${constructor_parameters} T *emboss_reserved_local_data,
::std::size_t emboss_reserved_local_size) {
return Generic${name}View</**/ ::emboss::support::ContiguousBuffer<T, 1, 0>>(
Expand All @@ -239,7 +240,7 @@ Make${name}View(${constructor_parameters} T *emboss_reserved_local_data,
}

template <typename T, ::std::size_t kAlignment>
inline Generic${name}View<
[[nodiscard]] inline Generic${name}View<
/**/ ::emboss::support::ContiguousBuffer<T, kAlignment, 0>>
MakeAligned${name}View(
${constructor_parameters} T *emboss_reserved_local_data,
Expand Down Expand Up @@ -374,28 +375,31 @@ ${write_fields}
}

// ** constant_structure_size_method ** ////////////////////////////////////////
static constexpr ::std::size_t SizeIn${units}() {
[[nodiscard]] static constexpr ::std::size_t SizeIn${units}() {
return static_cast</**/ ::std::size_t>(IntrinsicSizeIn${units}().Read());
}
static constexpr bool SizeIsKnown() {
[[nodiscard]] static constexpr bool SizeIsKnown() {
return IntrinsicSizeIn${units}().Ok();
}

// ** runtime_structure_size_method ** /////////////////////////////////////////
::std::size_t SizeIn${units}() const {
[[nodiscard]] ::std::size_t SizeIn${units}() const {
return static_cast</**/ ::std::size_t>(IntrinsicSizeIn${units}().Read());
}
bool SizeIsKnown() const { return IntrinsicSizeIn${units}().Ok(); }
[[nodiscard]] bool SizeIsKnown() const { return IntrinsicSizeIn${units}().Ok(); }


// ** ok_method_test ** ////////////////////////////////////////////////////////
// If we don't have enough information to determine whether ${field} is
// present in the structure, then structure.Ok() should be false.
if (!has_${field}.Known()) return false;
// If ${field} is present, but not Ok(), then structure.Ok() should be
// false. If ${field} is not present, it does not matter whether it is
// Ok().
if (has_${field}.ValueOrDefault() && !${field}.Ok()) return false;
{
const auto emboss_reserved_local_field_present = ${existence_condition};
if (!emboss_reserved_local_field_present.Known()) return false;
// If ${field} is present, but not Ok(), then structure.Ok() should be
// false. If ${field} is not present, it does not matter whether it is
// Ok().
if (emboss_reserved_local_field_present.ValueOrDefault() && !${field}.Ok()) return false;
}


// ** equals_method_test ** ////////////////////////////////////////////////////
Expand Down Expand Up @@ -487,8 +491,8 @@ struct ${name} {

// ** structure_single_field_method_declarations ** ////////////////////////////
${visibility}:
typename ${type_reader} ${name}() const;
::emboss::support::Maybe<bool> has_${name}() const;
[[nodiscard]] typename ${type_reader} ${name}() const;
[[nodiscard]] ::emboss::support::Maybe<bool> has_${name}() const;


// ** structure_single_field_method_definitions ** /////////////////////////////
Expand Down Expand Up @@ -543,9 +547,9 @@ Generic${parent_type}View<Storage>::has_${name}() const {
default;
~${virtual_view_type_name}() = default;

static constexpr ${logical_type} Read();
static constexpr ${logical_type} UncheckedRead();
static constexpr bool Ok() { return true; }
[[nodiscard]] static constexpr ${logical_type} Read();
[[nodiscard]] static constexpr ${logical_type} UncheckedRead();
[[nodiscard]] static constexpr bool Ok() { return true; }
template <class Stream>
void WriteToTextStream(Stream *emboss_reserved_local_stream,
const ::emboss::TextOutputOptions
Expand All @@ -554,13 +558,13 @@ Generic${parent_type}View<Storage>::has_${name}() const {
this, emboss_reserved_local_stream, emboss_reserved_local_options);
}

static constexpr bool IsAggregate() { return false; }
[[nodiscard]] static constexpr bool IsAggregate() { return false; }
};

static constexpr ${virtual_view_type_name} ${name}() {
[[nodiscard]] static constexpr ${virtual_view_type_name} ${name}() {
return ${virtual_view_type_name}();
}
static constexpr ::emboss::support::Maybe<bool> has_${name}() {
[[nodiscard]] static constexpr ::emboss::support::Maybe<bool> has_${name}() {
return ::emboss::support::Maybe<bool>(true);
}

Expand Down Expand Up @@ -603,14 +607,14 @@ Generic${parent_type}View<
default;
~${virtual_view_type_name}() = default;

${logical_type} Read() const {
[[nodiscard]] ${logical_type} Read() const {
EMBOSS_CHECK(view_.has_${name}().ValueOr(false));
auto emboss_reserved_local_value = MaybeRead();
EMBOSS_CHECK(emboss_reserved_local_value.Known());
EMBOSS_CHECK(ValueIsOk(emboss_reserved_local_value.ValueOrDefault()));
return emboss_reserved_local_value.ValueOrDefault();
}
${logical_type} UncheckedRead() const {
[[nodiscard]] ${logical_type} UncheckedRead() const {
// UncheckedRead() on a virtual still calls Ok() on its dependencies;
// i.e., it still does some bounds checking. This is because of a subtle
// case, illustrated by the example below:
Expand Down Expand Up @@ -644,7 +648,7 @@ Generic${parent_type}View<
}
// Ok() can be false if some dependency is unreadable, *or* if there is an
// error somewhere in the arithmetic -- say, division by zero.
bool Ok() const {
[[nodiscard]] bool Ok() const {
auto emboss_reserved_local_value = MaybeRead();
return emboss_reserved_local_value.Known() &&
ValueIsOk(emboss_reserved_local_value.ValueOrDefault());
Expand All @@ -657,30 +661,30 @@ Generic${parent_type}View<
this, emboss_reserved_local_stream, emboss_reserved_local_options);
}

static constexpr bool IsAggregate() { return false; }
[[nodiscard]] static constexpr bool IsAggregate() { return false; }

${write_methods}

private:
::emboss::support::Maybe</**/ ${logical_type}> MaybeRead() const {
[[nodiscard]] ::emboss::support::Maybe</**/ ${logical_type}> MaybeRead() const {
${read_subexpressions}
return ${read_value};
}

static constexpr bool ValueIsOk(
[[nodiscard]] static constexpr bool ValueIsOk(
${logical_type} emboss_reserved_local_value) {
return (void)emboss_reserved_local_value, // Silence -Wunused-parameter
${value_is_ok}.ValueOr(false);
}

const Generic${parent_type}View view_;
};
${virtual_view_type_name} ${name}() const;
::emboss::support::Maybe<bool> has_${name}() const;
[[nodiscard]] ${virtual_view_type_name} ${name}() const;
[[nodiscard]] ::emboss::support::Maybe<bool> has_${name}() const;


// ** structure_single_virtual_field_write_methods ** //////////////////////////
bool TryToWrite(${logical_type} emboss_reserved_local_value) {
[[nodiscard]] bool TryToWrite(${logical_type} emboss_reserved_local_value) {
const auto emboss_reserved_local_maybe_new_value = ${transform};
if (!CouldWriteValue(emboss_reserved_local_value)) return false;
return view_.${destination}.TryToWrite(
Expand All @@ -694,7 +698,7 @@ ${read_subexpressions}
void UncheckedWrite(${logical_type} emboss_reserved_local_value) {
view_.${destination}.UncheckedWrite((${transform}).ValueOrDefault());
}
bool CouldWriteValue(${logical_type} emboss_reserved_local_value) {
[[nodiscard]] bool CouldWriteValue(${logical_type} emboss_reserved_local_value) {
if (!ValueIsOk(emboss_reserved_local_value)) return false;
const auto emboss_reserved_local_maybe_new_value = ${transform};
if (!emboss_reserved_local_maybe_new_value.Known()) return false;
Expand Down Expand Up @@ -727,11 +731,11 @@ Generic${parent_type}View<Storage>::has_${name}() const {
// ** structure_single_field_indirect_method_declarations ** ///////////////////
${visibility}:
// The "this->" is required for (some versions of?) GCC.
auto ${name}() const -> decltype(this->${aliased_field}) {
[[nodiscard]] auto ${name}() const -> decltype(this->${aliased_field}) {
return has_${name}().ValueOrDefault() ? ${aliased_field}
: decltype(this->${aliased_field})();
}
::emboss::support::Maybe<bool> has_${name}() const;
[[nodiscard]] ::emboss::support::Maybe<bool> has_${name}() const;


// ** struct_single_field_indirect_method_definitions ** ///////////////////////
Expand All @@ -745,14 +749,14 @@ Generic${parent_type}View<Storage>::has_${name}() const {
// ** structure_single_parameter_field_method_declarations ** //////////////////
private:
// TODO(bolms): Is there any harm if these are public methods?
constexpr ::emboss::support::MaybeConstantView</**/ ${logical_type}>
[[nodiscard]] constexpr ::emboss::support::MaybeConstantView</**/ ${logical_type}>
${name}() const {
return parameters_initialized_
? ::emboss::support::MaybeConstantView</**/ ${logical_type}>(
${name}_)
: ::emboss::support::MaybeConstantView</**/ ${logical_type}>();
}
constexpr ::emboss::support::Maybe<bool> has_${name}() const {
[[nodiscard]] constexpr ::emboss::support::Maybe<bool> has_${name}() const {
return ::emboss::support::Maybe<bool>(parameters_initialized_);
}

Expand Down
Loading
Loading