pw_protobuf: Codegen kConstantCase field enums
pw_protobuf generated code includes an "enum class Fields : uint32_t"
type in in message, with values equal to the IDs for each field in the
message.
Unfortunately, the existing generated code uses SNAKE_CASE for the
names, rather than kConstantCase. In addition to be against the
style guide, this makes collisions with legacy C macros much more
likely.
Indeed, I ran into this issue due to a naming collison on OUTPUT_TYPE
between stm32cube.h and descriptor.pwpb.h. Of course, ST deserves
significant blame here for using inadequately namespaced global
identifiers... it's not just pw_protobuf's fault for not using
kConstantCase.
This commit does not yet migrate pigweed code to use the kConstantCase
values, that will be in an immediate follow-on. However, this commit
DOES add a GN variable to control generation of the legacy
names (in addition to new names):
pw_protobuf_compiler_GENERATE_LEGACY_ENUM_SNAKE_CASE_NAMES
This currently defaults to true, but will change to default to false
after the follow-on commit which migrates upstream pigweed away from
SNAKE_CASE. That will have to be done by an upstream pigweed developer.
Bug: b/266298474
Change-Id: I460a7d203905d39d234d3bf1a7621b52b188c109
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/126804
Commit-Queue: Wyatt Hepler <hepler@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
diff --git a/pw_protobuf/docs.rst b/pw_protobuf/docs.rst
index 7ebfb14..2bbcda3 100644
--- a/pw_protobuf/docs.rst
+++ b/pw_protobuf/docs.rst
@@ -373,15 +373,15 @@
while ((status = decoder.Next()).ok()) {
switch (decoder.Field().value()) {
- case Customer::Fields::AGE: {
+ case Customer::Fields::kAge: {
PW_TRY_ASSIGN(age, decoder.ReadAge());
break;
}
- case Customer::Fields::NAME: {
+ case Customer::Fields::kName: {
PW_TRY(decoder.ReadName(name));
break;
}
- case Customer::Fields::STATUS: {
+ case Customer::Fields::kStatus: {
PW_TRY_ASSIGN(status, decoder.ReadStatus());
break;
}
@@ -391,6 +391,15 @@
return status.IsOutOfRange() ? OkStatus() : status;
}
+.. warning:: ``Fields::SNAKE_CASE`` is deprecated. Use ``Fields::kCamelCase``.
+
+ Transitional support for ``Fields::SNAKE_CASE`` will soon only be available by
+ explicitly setting the following GN variable in your project:
+ ``pw_protobuf_compiler_GENERATE_LEGACY_ENUM_SNAKE_CASE_NAMES=true``
+
+ This support will be removed after downstream projects have been migrated.
+
+
Direct Writers and Readers
==========================
The lowest level API is provided by the core C++ implementation, and requires
@@ -432,12 +441,12 @@
#include "example_protos/customer.pwpb.h"
Status EncodeCustomer(pw::protobuf::StreamEncoder& encoder) {
- PW_TRY(encoder.WriteInt32(static_cast<uint32_t>(Customer::Fields::AGE),
+ PW_TRY(encoder.WriteInt32(static_cast<uint32_t>(Customer::Fields::kAge),
33));
- PW_TRY(encoder.WriteString(static_cast<uint32_t>(Customer::Fields::NAME),
+ PW_TRY(encoder.WriteString(static_cast<uint32_t>(Customer::Fields::kName),
"Joe Bloggs"sv));
PW_TRY(encoder.WriteUint32(
- static_cast<uint32_t>(Customer::Fields::STATUS),
+ static_cast<uint32_t>(Customer::Fields::kStatus),
static_cast<uint32_t>(Customer::Status::INACTIVE)));
}
@@ -475,15 +484,15 @@
while ((status = decoder.Next()).ok()) {
switch (decoder.FieldNumber().value()) {
- case static_cast<uint32_t>(Customer::Fields::AGE): {
+ case static_cast<uint32_t>(Customer::Fields::kAge): {
PW_TRY_ASSIGN(age, decoder.ReadInt32());
break;
}
- case static_cast<uint32_t>(Customer::Fields::NAME): {
+ case static_cast<uint32_t>(Customer::Fields::kName): {
PW_TRY(decoder.ReadString(name));
break;
}
- case static_cast<uint32_t>(Customer::Fields::STATUS): {
+ case static_cast<uint32_t>(Customer::Fields::kStatus): {
uint32_t status_value;
PW_TRY_ASSIGN(status_value, decoder.ReadUint32());
status = static_cast<Customer::Status>(status_value);
@@ -1318,7 +1327,7 @@
.. code:: c++
my_proto_encoder.WriteAge(42);
- my_proto_encoder.WriteInt32(static_cast<uint32_t>(MyProto::Fields::AGE), 42);
+ my_proto_encoder.WriteInt32(static_cast<uint32_t>(MyProto::Fields::kAge), 42);
Repeated Fields
---------------
@@ -1371,7 +1380,7 @@
my_proto_encoder.WriteNumbers(numbers);
my_proto_encoder.WritePackedInt32(
- static_cast<uint32_t>(MyProto::Fields::NUMBERS),
+ static_cast<uint32_t>(MyProto::Fields::kNumbers),
numbers);
Enumerations
@@ -1392,7 +1401,7 @@
my_proto_encoder.WriteAward(MyProto::Award::SILVER);
my_proto_encoder.WriteUint32(
- static_cast<uint32_t>(MyProto::Fields::AWARD),
+ static_cast<uint32_t>(MyProto::Fields::kAward),
static_cast<uint32_t>(MyProto::Award::SILVER));
Repeated Fields
@@ -1530,11 +1539,11 @@
// However, Field() is guaranteed to be valid after a call to Next()
// that returns OK, so the value can be used directly here.
switch (decoder.Field().value()) {
- case MyProto::Fields::AGE: {
+ case MyProto::Fields::kAge: {
PW_TRY_ASSIGN(age, decoder.ReadAge());
break;
}
- case MyProto::Fields::NAME:
+ case MyProto::Fields::kName:
// The string field is copied into the provided buffer. If the buffer
// is too small to fit the string, RESOURCE_EXHAUSTED is returned and
// the decoder is not advanced, allowing the field to be re-read.
@@ -1566,7 +1575,7 @@
Store::Message store{};
store.employees.SetDecoder([](Store::StreamDecoder& decoder) {
- PW_ASSERT(decoder.Field().value() == Store::Fields::EMPLOYEES);
+ PW_ASSERT(decoder.Field().value() == Store::Fields::kEmployees);
Employee::Message employee{};
// Set any callbacks on `employee`.
@@ -1604,7 +1613,7 @@
.. code:: c++
- case Owner::Fields::PET: {
+ case Owner::Fields::kPet: {
// Note that the parent decoder, owner_decoder, cannot be used until the
// nested decoder, pet_decoder, has been destroyed.
Animal::StreamDecoder pet_decoder = owner_decoder.GetPetDecoder();
@@ -1652,7 +1661,7 @@
.. code:: c++
PW_ASSERT(my_proto_decoder.FieldNumber().value() ==
- static_cast<uint32_t>(MyProto::Fields::AGE));
+ static_cast<uint32_t>(MyProto::Fields::kAge));
pw::Result<int32_t> my_proto_decoder.ReadInt32();
Repeated Fields
@@ -1718,7 +1727,7 @@
pw::Vector<int32_t, 8> numbers;
PW_ASSERT(my_proto_decoder.FieldNumber().value() ==
- static_cast<uint32_t>(MyProto::Fields::NUMBERS));
+ static_cast<uint32_t>(MyProto::Fields::kNumbers));
my_proto_decoder.ReadRepeatedInt32(numbers);
Enumerations
@@ -1758,7 +1767,7 @@
.. code-block:: c++
PW_ASSERT(my_proto_decoder.FieldNumber().value() ==
- static_cast<uint32_t>(MyProto::Fields::AWARD));
+ static_cast<uint32_t>(MyProto::Fields::kAward));
pw::Result<uint32_t> award_value = my_proto_decoder.ReadUint32();
if (award_value.ok()) {
MyProto::Award award = static_cast<MyProto::Award>(award_value);
diff --git a/pw_protobuf/py/pw_protobuf/codegen_pwpb.py b/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
index 2a3346b..d5eadaa 100644
--- a/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
+++ b/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
@@ -256,7 +256,8 @@
Result<{ctype}> ReadFoo({params...}) {
Result<uint32_t> field_number = FieldNumber();
PW_ASSERT(field_number.ok());
- PW_ASSERT(field_number.value() == static_cast<uint32_t>(Fields::FOO));
+ PW_ASSERT(field_number.value() ==
+ static_cast<uint32_t>(Fields::kFoo));
return decoder_->Read{type}({params...});
}
@@ -2315,7 +2316,10 @@
def forward_declare(
- node: ProtoMessage, root: ProtoNode, output: OutputFile
+ node: ProtoMessage,
+ root: ProtoNode,
+ output: OutputFile,
+ exclude_legacy_snake_case_field_name_enums: bool,
) -> None:
"""Generates code forward-declaring entities in a message's namespace."""
namespace = node.cpp_namespace(root=root)
@@ -2327,6 +2331,14 @@
with output.indent():
for field in node.fields():
output.write_line(f'{field.enum_name()} = {field.number()},')
+
+ # Migration support from SNAKE_CASE to kConstantCase.
+ if not exclude_legacy_snake_case_field_name_enums:
+ for field in node.fields():
+ output.write_line(
+ f'{field.legacy_enum_name()} = {field.number()},'
+ )
+
output.write_line('};')
# Declare the message's message struct.
@@ -2551,6 +2563,7 @@
package: ProtoNode,
output: OutputFile,
suppress_legacy_namespace: bool,
+ exclude_legacy_snake_case_field_name_enums: bool,
) -> None:
"""Generates code for a single .pb.h file corresponding to a .proto file."""
@@ -2593,7 +2606,12 @@
for node in package:
if node.type() == ProtoNode.Type.MESSAGE:
- forward_declare(cast(ProtoMessage, node), package, output)
+ forward_declare(
+ cast(ProtoMessage, node),
+ package,
+ output,
+ exclude_legacy_snake_case_field_name_enums,
+ )
# Define all top-level enums.
for node in package.children():
@@ -2677,7 +2695,10 @@
def process_proto_file(
- proto_file, proto_options, suppress_legacy_namespace: bool
+ proto_file,
+ proto_options,
+ suppress_legacy_namespace: bool,
+ exclude_legacy_snake_case_field_name_enums: bool,
) -> Iterable[OutputFile]:
"""Generates code for a single .proto file."""
@@ -2690,7 +2711,11 @@
output_filename = _proto_filename_to_generated_header(proto_file.name)
output_file = OutputFile(output_filename)
generate_code_for_package(
- proto_file, package_root, output_file, suppress_legacy_namespace
+ proto_file,
+ package_root,
+ output_file,
+ suppress_legacy_namespace,
+ exclude_legacy_snake_case_field_name_enums,
)
return [output_file]
diff --git a/pw_protobuf/py/pw_protobuf/plugin.py b/pw_protobuf/py/pw_protobuf/plugin.py
index 9315b76..bc3c405 100755
--- a/pw_protobuf/py/pw_protobuf/plugin.py
+++ b/pw_protobuf/py/pw_protobuf/plugin.py
@@ -53,6 +53,12 @@
help='If set, suppresses `using namespace` declarations, which '
'disallows use of the legacy non-prefixed namespace',
)
+ parser.add_argument(
+ '--exclude-legacy-snake-case-field-name-enums',
+ dest='exclude_legacy_snake_case_field_name_enums',
+ action='store_true',
+ help='Do not generate legacy SNAKE_CASE names for field name enums.',
+ )
# protoc passes the custom arguments in shell quoted form, separated by
# commas. Use shlex to split them, correctly handling quoted sections, with
@@ -87,6 +93,9 @@
proto_file,
proto_options,
suppress_legacy_namespace=args.no_legacy_namespace,
+ exclude_legacy_snake_case_field_name_enums=(
+ args.exclude_legacy_snake_case_field_name_enums
+ ),
)
for output_file in output_files:
fd = res.file.add()
diff --git a/pw_protobuf/py/pw_protobuf/proto_tree.py b/pw_protobuf/py/pw_protobuf/proto_tree.py
index 44c8804..15bfb73 100644
--- a/pw_protobuf/py/pw_protobuf/proto_tree.py
+++ b/pw_protobuf/py/pw_protobuf/proto_tree.py
@@ -505,6 +505,9 @@
return self._field_name
def enum_name(self) -> str:
+ return 'k' + self.name()
+
+ def legacy_enum_name(self) -> str:
return self.upper_snake_case(
symbol_name_mapping.fix_cc_enum_value_name(self._field_name)
)
diff --git a/pw_protobuf_compiler/proto.gni b/pw_protobuf_compiler/proto.gni
index 8a69770..8a62801 100644
--- a/pw_protobuf_compiler/proto.gni
+++ b/pw_protobuf_compiler/proto.gni
@@ -85,17 +85,21 @@
deps += invoker.other_deps
}
- args = [
- "--language",
- invoker.language,
- "--include-file",
- _includes[0],
- "--compile-dir",
- rebase_path(invoker.compile_dir, root_build_dir),
- "--out-dir",
- rebase_path(_out_dir, root_build_dir),
- "--sources",
- ] + rebase_path(invoker.sources, root_build_dir)
+ args = []
+ if (!pw_protobuf_compiler_GENERATE_LEGACY_ENUM_SNAKE_CASE_NAMES) {
+ args += [ "--exclude-pwpb-legacy-snake-case-field-name-enums" ]
+ }
+ args += [
+ "--language",
+ invoker.language,
+ "--include-file",
+ _includes[0],
+ "--compile-dir",
+ rebase_path(invoker.compile_dir, root_build_dir),
+ "--out-dir",
+ rebase_path(_out_dir, root_build_dir),
+ "--sources",
+ ] + rebase_path(invoker.sources, root_build_dir)
if (defined(invoker.plugin)) {
inputs = [ invoker.plugin ]
diff --git a/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py b/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py
index a87dcda..95e2605 100644
--- a/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py
+++ b/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py
@@ -81,6 +81,15 @@
action='store_false',
help='Do not generate pyi files for python',
)
+ parser.add_argument(
+ '--exclude-pwpb-legacy-snake-case-field-name-enums',
+ dest='exclude_pwpb_legacy_snake_case_field_name_enums',
+ action='store_true',
+ help=(
+ 'If set, generates legacy SNAKE_CASE names for field name enums '
+ 'in PWPB.'
+ ),
+ )
return parser
@@ -95,15 +104,27 @@
def protoc_pwpb_args(
args: argparse.Namespace, include_paths: List[str]
) -> Tuple[str, ...]:
- return (
+ out_args = [
'--plugin',
f'protoc-gen-custom={args.plugin_path}',
f'--custom_opt=-I{args.compile_dir}',
*[f'--custom_opt=-I{include_path}' for include_path in include_paths],
- '--custom_out',
- args.out_dir,
+ ]
+
+ if args.exclude_pwpb_legacy_snake_case_field_name_enums:
+ out_args.append(
+ '--custom_opt=--exclude-legacy-snake-case-field-name-enums'
+ )
+
+ out_args.extend(
+ [
+ '--custom_out',
+ args.out_dir,
+ ]
)
+ return tuple(out_args)
+
def protoc_pwpb_rpc_args(
args: argparse.Namespace, _include_paths: List[str]
diff --git a/pw_protobuf_compiler/toolchain.gni b/pw_protobuf_compiler/toolchain.gni
index 606e69d..806a963 100644
--- a/pw_protobuf_compiler/toolchain.gni
+++ b/pw_protobuf_compiler/toolchain.gni
@@ -20,4 +20,10 @@
# in a single toolchain to avoid unnecessary duplication in the build.
pw_protobuf_compiler_TOOLCHAIN =
"$dir_pw_protobuf_compiler/toolchain:protocol_buffer"
+
+ # pwpb previously generated field enum names in SNAKE_CASE rather than
+ # kConstantCase. Set this variable to temporarily enable legacy SNAKE_CASE
+ # support while you migrate your codebase to kConstantCase.
+ # b/266298474
+ pw_protobuf_compiler_GENERATE_LEGACY_ENUM_SNAKE_CASE_NAMES = true
}
