From 63067c92ceae1ad26a32dcd465a1e4a5845c4e26 Mon Sep 17 00:00:00 2001 From: Dave Allison Date: Wed, 26 Aug 2026 19:23:46 -0700 Subject: [PATCH] Support repeated ROS intrinsics, cross-package enums, aliased enums, and self-named fields Generating Phaser schemas for a large corpus of real ROS messages turned up four cases the generator could not express. Repeated time and duration. The ROS frontend rejected any repeated ROS intrinsic, so a `time[]` or `duration[]` field blocked the whole transitive proto set. RosRepeatedMessageField wraps the message vector or array and converts per element, so such a field reads and writes as ::ros::Time rather than the google.protobuf.Timestamp backing it. A repeated Header stays rejected: RosHeaderField hands out a view whose lifetime is tied to the field rather than a value, so it has no element type to repeat. An intrinsic in a oneof stays rejected too. Cross-package enums. Enums are emitted at namespace scope, so the short name only resolved inside the declaring package. EnumName now qualifies with the declaring package's namespace and the added namespace, as MessageName does. Aliased enums. `option allow_alias` lets several names share one number, which gave the stringizer a switch with duplicate case labels. Only the first name for a number gets a case now, matching protobuf's own _Name. Self-named fields. A .msg file may name a field after the message holding it, which C++ reads as a constructor rather than a member. Such a field now gets trailing underscores, as reserved words already did. --- phaser/BUILD.bazel | 49 ++++ phaser/aliased_enum_test.cc | 48 ++++ phaser/compiler/enum_gen.cc | 10 +- phaser/compiler/message_gen.cc | 141 +++++++++--- phaser/compiler/message_gen.h | 7 + phaser/cross_package_enum_ros_test.cc | 67 ++++++ phaser/cross_package_enum_test.cc | 86 +++++++ phaser/ros_intrinsics_test.cc | 96 ++++++++ phaser/ros_metadata_protobuf_frontend_test.cc | 4 +- phaser/ros_metadata_ros_frontend_test.cc | 3 +- phaser/ros_wire_conversion_test.cc | 42 ++++ phaser/runtime/ros.h | 209 ++++++++++++++++++ phaser/self_named_field_test.cc | 99 +++++++++ phaser/testdata/AliasedEnum.proto | 21 ++ phaser/testdata/BUILD | 71 +++++- phaser/testdata/CrossPackageEnum.proto | 27 +++ phaser/testdata/CrossPackageEnumUser.proto | 22 ++ phaser/testdata/InvalidRosIntrinsic.proto | 4 +- .../testdata/InvalidRosIntrinsicOneof.proto | 12 + phaser/testdata/RosIntrinsics.proto | 7 + .../RosIntrinsicsProtobufFrontend.proto | 7 + phaser/testdata/RosMetadata.proto | 1 + phaser/testdata/SelfNamedField.proto | 35 +++ phaser/testdata/invalid_ros_intrinsic_test.sh | 37 +++- 24 files changed, 1062 insertions(+), 43 deletions(-) create mode 100644 phaser/aliased_enum_test.cc create mode 100644 phaser/cross_package_enum_ros_test.cc create mode 100644 phaser/cross_package_enum_test.cc create mode 100644 phaser/self_named_field_test.cc create mode 100644 phaser/testdata/AliasedEnum.proto create mode 100644 phaser/testdata/CrossPackageEnum.proto create mode 100644 phaser/testdata/CrossPackageEnumUser.proto create mode 100644 phaser/testdata/InvalidRosIntrinsicOneof.proto create mode 100644 phaser/testdata/SelfNamedField.proto diff --git a/phaser/BUILD.bazel b/phaser/BUILD.bazel index 191547f..44ed55f 100644 --- a/phaser/BUILD.bazel +++ b/phaser/BUILD.bazel @@ -113,6 +113,55 @@ cc_test( ], ) +cc_test( + name = "self_named_field_test", + srcs = ["self_named_field_test.cc"], + copts = PHASER_COPTS, + data = ["valgrind.supp"], + deps = [ + "//phaser/runtime:phaser_runtime", + "//phaser/testdata:self_named_field_phaser", + "//phaser/testdata:self_named_field_ros_phaser", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "aliased_enum_test", + srcs = ["aliased_enum_test.cc"], + copts = PHASER_COPTS, + data = ["valgrind.supp"], + deps = [ + "//phaser/runtime:phaser_runtime", + "//phaser/testdata:aliased_enum_phaser", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "cross_package_enum_test", + srcs = ["cross_package_enum_test.cc"], + copts = PHASER_COPTS, + data = ["valgrind.supp"], + deps = [ + "//phaser/runtime:phaser_runtime", + "//phaser/testdata:cross_package_enum_phaser", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "cross_package_enum_ros_test", + srcs = ["cross_package_enum_ros_test.cc"], + copts = PHASER_COPTS, + data = ["valgrind.supp"], + deps = [ + "//phaser/runtime:phaser_runtime", + "//phaser/testdata:cross_package_enum_ros_phaser", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "ros_native_frontend_compatibility_test", srcs = ["ros_native_frontend_compatibility_test.cc"], diff --git a/phaser/aliased_enum_test.cc b/phaser/aliased_enum_test.cc new file mode 100644 index 0000000..1e1b644 --- /dev/null +++ b/phaser/aliased_enum_test.cc @@ -0,0 +1,48 @@ +#include + +#include "gtest/gtest.h" +#include "phaser/testdata/AliasedEnum.phaser.h" + +namespace alias::phaser { +namespace { + +TEST(AliasedEnumTest, AliasesCompareEqualToTheirPrimaryName) { + EXPECT_EQ(SIGNAL_UNSPECIFIED, SIGNAL_IDLE); + EXPECT_EQ(SIGNAL_BUSY, SIGNAL_WORKING); +} + +// Protobuf's own _Name reports the first name declared for a number, so the +// stringizer keeps that case and drops the aliases. +TEST(AliasedEnumTest, StringizerReportsTheFirstNameForANumber) { + EXPECT_EQ("SIGNAL_UNSPECIFIED", Signal_Name(SIGNAL_IDLE)); + EXPECT_EQ("SIGNAL_BUSY", Signal_Name(SIGNAL_WORKING)); + EXPECT_EQ("SIGNAL_DONE", Signal_Name(SIGNAL_DONE)); +} + +TEST(AliasedEnumTest, ParserAcceptsEveryAlias) { + Signal parsed; + Signal_Parse("SIGNAL_IDLE", &parsed); + EXPECT_EQ(SIGNAL_IDLE, parsed); + + Signal_Parse("SIGNAL_WORKING", &parsed); + EXPECT_EQ(SIGNAL_BUSY, parsed); +} + +TEST(AliasedEnumTest, FieldsRoundTrip) { + Job job; + job.set_signal(SIGNAL_WORKING); + job.add_history(SIGNAL_IDLE); + job.add_history(SIGNAL_DONE); + + const std::string wire = job.SerializeAsString(); + + Job parsed; + ASSERT_TRUE(parsed.ParseFromString(wire)); + EXPECT_EQ(SIGNAL_BUSY, parsed.signal()); + ASSERT_EQ(2u, parsed.history_size()); + EXPECT_EQ(SIGNAL_UNSPECIFIED, parsed.history(0)); + EXPECT_EQ(SIGNAL_DONE, parsed.history(1)); +} + +} // namespace +} // namespace alias::phaser diff --git a/phaser/compiler/enum_gen.cc b/phaser/compiler/enum_gen.cc index e229ce9..d87020c 100644 --- a/phaser/compiler/enum_gen.cc +++ b/phaser/compiler/enum_gen.cc @@ -5,6 +5,7 @@ #include "phaser/compiler/enum_gen.h" #include +#include namespace phaser { @@ -24,12 +25,19 @@ void EnumGenerator::GenerateHeader(std::ostream& os) { } os << "};\n\n"; - // Stringizer + // Stringizer. With `option allow_alias`, several names share one number, so + // only the first is given a case label; a switch cannot repeat one. That + // matches protobuf's own _Name, which also reports the first name declared + // for a number. os << "struct " << name << "Stringizer {\n"; os << " std::string operator()(" << name << " e) {\n"; os << " switch (e) {\n"; + std::set stringized_numbers; for (int i = 0; i < enum_->value_count(); i++) { const google::protobuf::EnumValueDescriptor* value = enum_->value(i); + if (!stringized_numbers.insert(value->number()).second) { + continue; + } std::string const_name(value->name()); if (enum_->containing_type() != nullptr) { const_name = name + "_" + const_name; diff --git a/phaser/compiler/message_gen.cc b/phaser/compiler/message_gen.cc index 59233d7..a2221fc 100644 --- a/phaser/compiler/message_gen.cc +++ b/phaser/compiler/message_gen.cc @@ -361,12 +361,39 @@ std::string MessageGenerator::SanitizedIdentifier( return name; } +std::string MessageGenerator::GeneratedClassName() const { + std::string name(message_->name()); + if (message_->containing_type() != nullptr) { + name = std::string(message_->containing_type()->name()) + "_" + name; + } + return name; +} + +// A field is free to carry a name C++ will not take verbatim: a reserved word, +// or the name of the message holding it, which would turn the accessor into a +// constructor. Both get trailing underscores, as many as it takes to keep the +// accessor clear of the class name. protoc never meets the second case because +// it lowercases accessors, and roscpp never meets it because it names the +// struct `Foo_` and typedefs `Foo` to that. +std::string MessageGenerator::FieldAccessorName( + const std::string& proto_name) const { + std::string name = SanitizedIdentifier(proto_name); + while (name == GeneratedClassName()) { + name += "_"; + } + return name; +} + std::string MessageGenerator::MemberVariableName( const std::string& proto_name) const { + // The ROS frontend exposes the member itself, so there is no accessor for it + // to collide with. The protobuf frontend keeps the accessor at the field name + // and pushes the member one underscore past it. + std::string name = FieldAccessorName(proto_name); if (IsRosFrontend()) { - return SanitizedIdentifier(proto_name); + return name; } - return proto_name + "_"; + return name + "_"; } std::string MessageGenerator::OneofVariantTypeName( @@ -449,10 +476,20 @@ absl::Status MessageGenerator::ValidateFieldOptions() const { return status; } if (IsRosFrontend() && IsRosIntrinsic(field) && - (field->is_repeated() || field->containing_oneof() != nullptr)) { + field->containing_oneof() != nullptr) { return absl::InvalidArgumentError(absl::StrFormat( - "ROS intrinsic field %s.%s must be singular and cannot be in a " - "oneof", + "ROS intrinsic field %s.%s cannot be in a oneof", + message_->full_name(), field->name())); + } + // time[] and duration[] are repeated through RosRepeatedMessageField, which + // converts per element. A repeated Header would need the same treatment, + // but RosHeaderField hands out a view whose lifetime is tied to the field + // rather than a value, so it has no element type to repeat. + if (IsRosFrontend() && IsRosIntrinsic(field) && field->is_repeated() && + IsRosHeader(field->message_type())) { + return absl::InvalidArgumentError(absl::StrFormat( + "ROS intrinsic field %s.%s must be singular: the ROS frontend does " + "not support a repeated Header", message_->full_name(), field->name())); } } @@ -559,7 +596,19 @@ std::string MessageGenerator::EnumName( if (desc->containing_type() != nullptr) { name = std::string(desc->containing_type()->name()) + "_" + name; } - return name; + // Enums, along with their stringizer and parser, are emitted at namespace + // scope, so the short name only resolves inside the package that declared + // them. A field referring to an enum from another package has to spell out + // that package's namespace and the added namespace, if any. + std::string enum_package(desc->file()->package()); + if (enum_package == package_name_) { + return name; + } + std::string scope = absl::StrReplaceAll(enum_package, {{".", "::"}}); + if (!added_namespace_.empty()) { + scope += "::" + added_namespace_; + } + return scope + "::" + name; } std::string MessageGenerator::MessageName( @@ -775,6 +824,9 @@ std::string MessageGenerator::FieldRepeatedVectorCType( case google::protobuf::FieldDescriptor::TYPE_BYTES: return "StringVectorField"; case google::protobuf::FieldDescriptor::TYPE_MESSAGE: + if (IsRosFrontend() && IsRosIntrinsic(field)) { + return RosIntrinsicVectorFieldType(field); + } return "MessageVectorField<" + MessageName(field->message_type(), true) + ">"; case google::protobuf::FieldDescriptor::TYPE_GROUP: @@ -834,6 +886,9 @@ std::string MessageGenerator::FieldRepeatedArrayCType( case google::protobuf::FieldDescriptor::TYPE_BYTES: return "StringArrayField<" + extent + ">"; case google::protobuf::FieldDescriptor::TYPE_MESSAGE: + if (IsRosFrontend() && IsRosIntrinsic(field)) { + return RosIntrinsicArrayFieldType(field, extent); + } return "MessageArrayField<" + MessageName(field->message_type(), true) + ", " + extent + ">"; case google::protobuf::FieldDescriptor::TYPE_GROUP: @@ -970,6 +1025,28 @@ std::string MessageGenerator::RosIntrinsicFieldType( return "RosHeaderField<" + backend + ">"; } +std::string MessageGenerator::RosIntrinsicVectorFieldType( + const google::protobuf::FieldDescriptor* field) { + const std::string backend = MessageName(field->message_type(), true); + if (IsRosTime(field->message_type())) { + return "RosTimeVectorField<" + backend + ">"; + } + // A repeated Header is rejected in ValidateFieldOptions. + assert(IsRosDuration(field->message_type())); + return "RosDurationVectorField<" + backend + ">"; +} + +std::string MessageGenerator::RosIntrinsicArrayFieldType( + const google::protobuf::FieldDescriptor* field, const std::string& extent) { + const std::string backend = MessageName(field->message_type(), true); + if (IsRosTime(field->message_type())) { + return "RosTimeArrayField<" + backend + ", " + extent + ">"; + } + // A repeated Header is rejected in ValidateFieldOptions. + assert(IsRosDuration(field->message_type())); + return "RosDurationArrayField<" + backend + ", " + extent + ">"; +} + std::string MessageGenerator::RosIntrinsicCType( const google::protobuf::FieldDescriptor* field) { if (IsRosTime(field->message_type())) { @@ -1914,8 +1991,7 @@ void MessageGenerator::GenerateFieldProtobufAccessors( std::shared_ptr field, std::shared_ptr union_field, int union_index, std::ostream& os) { std::string field_name(field->field->name()); - std::string sanitized_field_name = - field_name + +(IsCppReservedWord(field_name) ? "_" : ""); + std::string sanitized_field_name = FieldAccessorName(field_name); std::string member_name = field->member_name; if (union_field != nullptr) { @@ -3934,6 +4010,11 @@ void MessageGenerator::GenerateStreamer(std::ostream& os) { google::protobuf::FieldDescriptor::TYPE_BYTES) { os << " os << \"" << field->field->name() << ": \\\"\" << v << \"\\\"\" << std::endl;\n"; + } else if (IsRosFrontend() && IsRosIntrinsic(field->field)) { + // Like a singular message field, no colon after the name. + os << " os << \"" << field->field->name() << " \";\n"; + os << " msg." << field->member_name << ".PrintElement(os, v);\n"; + os << " os << std::endl;\n"; } else { os << " os << \"" << field->field->name() << ": \" << v << std::endl;\n"; @@ -3961,8 +4042,11 @@ void MessageGenerator::GenerateStreamer(std::ostream& os) { void MessageGenerator::GenerateCopy(std::ostream& os, bool decl) { if (decl) { - os << " template \n"; - os << " absl::Status CloneFrom(const T& _phaser_other);\n\n"; + // The template parameter carries the _phaser_ prefix for the same reason + // the locals below do: a field is free to be named T, and inside the + // template the parameter would win the name lookup. + os << " template \n"; + os << " absl::Status CloneFrom(const _phaser_Source& _phaser_other);\n\n"; os << " void CopyFrom(" "const ::phaser::Message& _phaser_other) override {\n"; os << " const " << MessageName(message_) @@ -3974,17 +4058,21 @@ void MessageGenerator::GenerateCopy(std::ostream& os, bool decl) { } // CloneFrom. - os << "template \n"; + os << "template \n"; os << "inline absl::Status " << MessageName(message_) - << "::CloneFrom([[maybe_unused]] const T& _phaser_other) {\n"; + << "::CloneFrom([[maybe_unused]] const _phaser_Source& _phaser_other) " + "{\n"; if (IsRosFrontend()) { for (auto& field : fields_) { if (field->field->is_repeated()) { os << " " << field->member_name << ".Clear();\n"; if (UsesArrayFacade(field->field)) { const int array_size = GetArraySize(field->field); + // A ROS intrinsic element is a value, not a bound message, so it + // clones through Set/Get like a string rather than through CloneFrom. if (field->field->type() == - google::protobuf::FieldDescriptor::TYPE_MESSAGE) { + google::protobuf::FieldDescriptor::TYPE_MESSAGE && + !IsRosIntrinsic(field->field)) { os << " for (size_t _phaser_index = 0; " "_phaser_index < static_cast(" << array_size << "); ++_phaser_index) {\n"; @@ -4019,7 +4107,8 @@ void MessageGenerator::GenerateCopy(std::ostream& os, bool decl) { os << " }\n"; } } else if (field->field->type() == - google::protobuf::FieldDescriptor::TYPE_MESSAGE) { + google::protobuf::FieldDescriptor::TYPE_MESSAGE && + !IsRosIntrinsic(field->field)) { os << " for (auto _phaser_value : _phaser_other." << field->member_name << ") {\n"; os << " auto _phaser_message = " << field->member_name @@ -4103,18 +4192,21 @@ void MessageGenerator::GenerateCopy(std::ostream& os, bool decl) { } } else { for (auto& field : fields_) { + // Only the bare accessor carries the sanitizing; a prefixed one such as + // `set_x` is already a legal name whatever the field is called. + const std::string name(field->field->name()); + const std::string accessor = FieldAccessorName(name); if (field->field->is_repeated()) { - os << " for (auto _phaser_value : _phaser_other." - << field->field->name() << "()) {\n"; + os << " for (auto _phaser_value : _phaser_other." << accessor + << "()) {\n"; if (field->field->type() == google::protobuf::FieldDescriptor::TYPE_MESSAGE) { - os << " auto _phaser_message = add_" << field->field->name() - << "();\n"; + os << " auto _phaser_message = add_" << name << "();\n"; os << " if (absl::Status _phaser_status = " "_phaser_message.CloneFrom(_phaser_value); " "!_phaser_status.ok()) return _phaser_status;\n"; } else { - os << " add_" << field->field->name() << "(_phaser_value);\n"; + os << " add_" << name << "(_phaser_value);\n"; } os << " }\n"; @@ -4123,15 +4215,14 @@ void MessageGenerator::GenerateCopy(std::ostream& os, bool decl) { << ".IsPresent()) {\n"; if (field->field->type() == google::protobuf::FieldDescriptor::TYPE_MESSAGE) { - os << " auto* _phaser_message = mutable_" << field->field->name() - << "();\n"; + os << " auto* _phaser_message = mutable_" << name << "();\n"; os << " if (absl::Status _phaser_status = " "_phaser_message->CloneFrom(_phaser_other." - << field->field->name() + << accessor << "()); !_phaser_status.ok()) return _phaser_status;\n"; } else { - os << " set_" << field->field->name() << "(_phaser_other." - << field->field->name() << "());\n"; + os << " set_" << name << "(_phaser_other." << accessor + << "());\n"; } os << " }\n"; } @@ -4145,7 +4236,7 @@ void MessageGenerator::GenerateCopy(std::ostream& os, bool decl) { os << " case " << field->field->number() << ":\n"; os << " if (absl::Status _phaser_status = " << u->member_name << ".template CloneFrom<" << i << ">(_phaser_other." - << field->field->name() + << FieldAccessorName(std::string(field->field->name())) << "()); !_phaser_status.ok()) return _phaser_status;\n"; os << " break;\n"; } diff --git a/phaser/compiler/message_gen.h b/phaser/compiler/message_gen.h index 3e74bed..5495fdd 100644 --- a/phaser/compiler/message_gen.h +++ b/phaser/compiler/message_gen.h @@ -193,6 +193,8 @@ class MessageGenerator { uint32_t FieldBinarySize(const google::protobuf::FieldDescriptor* field); std::string FieldInfoType(const google::protobuf::FieldDescriptor* field); std::string SanitizedIdentifier(const std::string& name) const; + std::string GeneratedClassName() const; + std::string FieldAccessorName(const std::string& name) const; std::string MemberVariableName(const std::string& proto_name) const; std::string OneofVariantTypeName( const google::protobuf::OneofDescriptor* oneof) const; @@ -206,6 +208,11 @@ class MessageGenerator { bool IsRosIntrinsic(const google::protobuf::FieldDescriptor* field) const; std::string RosIntrinsicFieldType( const google::protobuf::FieldDescriptor* field); + std::string RosIntrinsicVectorFieldType( + const google::protobuf::FieldDescriptor* field); + std::string RosIntrinsicArrayFieldType( + const google::protobuf::FieldDescriptor* field, + const std::string& extent); std::string RosIntrinsicCType( const google::protobuf::FieldDescriptor* field); absl::Status ValidateFieldOptions() const; diff --git a/phaser/cross_package_enum_ros_test.cc b/phaser/cross_package_enum_ros_test.cc new file mode 100644 index 0000000..684886c --- /dev/null +++ b/phaser/cross_package_enum_ros_test.cc @@ -0,0 +1,67 @@ +#include + +#include "gtest/gtest.h" +#include "phaser/testdata/cross_package_enum_ros_phaser/phaser/testdata/CrossPackageEnumUser.phaser.h" + +namespace canvas::phaser_ros { +namespace { + +using ::palette::phaser_ros::Color; +using ::palette::phaser_ros::COLOR_BLUE; +using ::palette::phaser_ros::COLOR_GREEN; +using ::palette::phaser_ros::COLOR_RED; +using ::palette::phaser_ros::Shade_Depth_DEPTH_DARK; + +TEST(CrossPackageEnumRosTest, SingularFieldRoundTrips) { + Drawing drawing; + drawing.background = COLOR_BLUE; + EXPECT_EQ(COLOR_BLUE, drawing.background); +} + +TEST(CrossPackageEnumRosTest, NestedEnumFieldRoundTrips) { + Drawing drawing; + drawing.depth = Shade_Depth_DEPTH_DARK; + EXPECT_EQ(Shade_Depth_DEPTH_DARK, drawing.depth); +} + +TEST(CrossPackageEnumRosTest, RepeatedFieldRoundTrips) { + Drawing drawing; + drawing.strokes.push_back(COLOR_RED); + drawing.strokes.push_back(COLOR_GREEN); + ASSERT_EQ(2u, drawing.strokes.size()); + EXPECT_EQ(COLOR_RED, drawing.strokes[0]); + EXPECT_EQ(COLOR_GREEN, drawing.strokes[1]); +} + +TEST(CrossPackageEnumRosTest, FixedArrayFieldRoundTrips) { + Drawing drawing; + drawing.corners[0] = COLOR_GREEN; + drawing.corners[3] = COLOR_BLUE; + EXPECT_EQ(COLOR_GREEN, drawing.corners[0]); + EXPECT_EQ(COLOR_BLUE, drawing.corners[3]); +} + +TEST(CrossPackageEnumRosTest, StringizerAndParserResolveAcrossPackages) { + EXPECT_EQ("COLOR_GREEN", ::palette::phaser_ros::Color_Name(COLOR_GREEN)); + + Color parsed; + ::palette::phaser_ros::Color_Parse("COLOR_BLUE", &parsed); + EXPECT_EQ(COLOR_BLUE, parsed); +} + +TEST(CrossPackageEnumRosTest, SerializesAndParses) { + Drawing drawing; + drawing.background = COLOR_BLUE; + drawing.strokes.push_back(COLOR_RED); + + const std::string wire = drawing.SerializeAsString(); + + Drawing parsed; + ASSERT_TRUE(parsed.ParseFromString(wire)); + EXPECT_EQ(COLOR_BLUE, parsed.background); + ASSERT_EQ(1u, parsed.strokes.size()); + EXPECT_EQ(COLOR_RED, parsed.strokes[0]); +} + +} // namespace +} // namespace canvas::phaser_ros diff --git a/phaser/cross_package_enum_test.cc b/phaser/cross_package_enum_test.cc new file mode 100644 index 0000000..823d76a --- /dev/null +++ b/phaser/cross_package_enum_test.cc @@ -0,0 +1,86 @@ +#include + +#include "gtest/gtest.h" +#include "phaser/testdata/CrossPackageEnumUser.phaser.h" + +namespace canvas::phaser { +namespace { + +using ::palette::phaser::Color; +using ::palette::phaser::COLOR_BLUE; +using ::palette::phaser::COLOR_GREEN; +using ::palette::phaser::COLOR_RED; +using ::palette::phaser::Shade_Depth; +using ::palette::phaser::Shade_Depth_DEPTH_DARK; + +TEST(CrossPackageEnumTest, SingularFieldRoundTrips) { + Drawing drawing; + drawing.set_background(COLOR_BLUE); + EXPECT_EQ(COLOR_BLUE, drawing.background()); +} + +TEST(CrossPackageEnumTest, NestedEnumFieldRoundTrips) { + Drawing drawing; + drawing.set_depth(Shade_Depth_DEPTH_DARK); + EXPECT_EQ(Shade_Depth_DEPTH_DARK, drawing.depth()); +} + +TEST(CrossPackageEnumTest, RepeatedFieldRoundTrips) { + Drawing drawing; + drawing.add_strokes(COLOR_RED); + drawing.add_strokes(COLOR_GREEN); + ASSERT_EQ(2u, drawing.strokes_size()); + EXPECT_EQ(COLOR_RED, drawing.strokes(0)); + EXPECT_EQ(COLOR_GREEN, drawing.strokes(1)); +} + +// The protobuf frontend ignores (phaser.array_size) and keeps the field a +// vector, so size it before indexing into it. +TEST(CrossPackageEnumTest, SizedArrayFieldRoundTrips) { + Drawing drawing; + drawing.resize_corners(4); + drawing.set_corners(0, COLOR_GREEN); + drawing.set_corners(3, COLOR_BLUE); + EXPECT_EQ(COLOR_GREEN, drawing.corners(0)); + EXPECT_EQ(COLOR_BLUE, drawing.corners(3)); +} + +TEST(CrossPackageEnumTest, UnionFieldRoundTrips) { + Drawing drawing; + drawing.set_accent_color(COLOR_BLUE); + EXPECT_EQ(COLOR_BLUE, drawing.accent_color()); + EXPECT_TRUE(drawing.has_accent_color()); +} + +TEST(CrossPackageEnumTest, StringizerAndParserResolveAcrossPackages) { + EXPECT_EQ("COLOR_GREEN", ::palette::phaser::Color_Name(COLOR_GREEN)); + + Color parsed; + ::palette::phaser::Color_Parse("COLOR_BLUE", &parsed); + EXPECT_EQ(COLOR_BLUE, parsed); +} + +TEST(CrossPackageEnumTest, DebugStringNamesTheEnumValue) { + Drawing drawing; + drawing.set_background(COLOR_GREEN); + EXPECT_NE(std::string::npos, drawing.DebugString().find("COLOR_GREEN")); +} + +TEST(CrossPackageEnumTest, SerializesAndParses) { + Drawing drawing; + drawing.set_background(COLOR_BLUE); + drawing.add_strokes(COLOR_RED); + drawing.set_depth(Shade_Depth_DEPTH_DARK); + + const std::string wire = drawing.SerializeAsString(); + + Drawing parsed; + ASSERT_TRUE(parsed.ParseFromString(wire)); + EXPECT_EQ(COLOR_BLUE, parsed.background()); + ASSERT_EQ(1u, parsed.strokes_size()); + EXPECT_EQ(COLOR_RED, parsed.strokes(0)); + EXPECT_EQ(Shade_Depth_DEPTH_DARK, parsed.depth()); +} + +} // namespace +} // namespace canvas::phaser diff --git a/phaser/ros_intrinsics_test.cc b/phaser/ros_intrinsics_test.cc index ab7c75f..fe98490 100644 --- a/phaser/ros_intrinsics_test.cc +++ b/phaser/ros_intrinsics_test.cc @@ -108,5 +108,101 @@ TEST(RosIntrinsicsTest, CopyAndMovePreserveDeferredValues) { EXPECT_EQ(ReadFrame(moved.header), "map"); } +void FillRepeated(RosIntrinsicMessage& message) { + message.stamps.Add(::ros::Time(1, 2)); + message.stamps.Add(::ros::Time(3, 4)); + message.timeouts.Add(::ros::Duration(-5, 6)); + message.fixed_stamps.Set(0, ::ros::Time(7, 8)); + message.fixed_stamps.Set(1, ::ros::Time(9, 10)); + message.fixed_timeouts.Set(0, ::ros::Duration(-11, 12)); + message.fixed_timeouts.Set(1, ::ros::Duration(13, 14)); +} + +void ExpectRepeated(const RosIntrinsicMessage& message) { + ASSERT_EQ(message.stamps.size(), 2u); + EXPECT_EQ(message.stamps.Get(0), ::ros::Time(1, 2)); + EXPECT_EQ(message.stamps.Get(1), ::ros::Time(3, 4)); + ASSERT_EQ(message.timeouts.size(), 1u); + EXPECT_EQ(message.timeouts.Get(0), ::ros::Duration(-5, 6)); + ASSERT_EQ(message.fixed_stamps.size(), 2u); + EXPECT_EQ(message.fixed_stamps.Get(0), ::ros::Time(7, 8)); + EXPECT_EQ(message.fixed_stamps.Get(1), ::ros::Time(9, 10)); + ASSERT_EQ(message.fixed_timeouts.size(), 2u); + EXPECT_EQ(message.fixed_timeouts.Get(0), ::ros::Duration(-11, 12)); + EXPECT_EQ(message.fixed_timeouts.Get(1), ::ros::Duration(13, 14)); +} + +TEST(RosIntrinsicsTest, RepeatedIntrinsicsReadAsRosValues) { + RosIntrinsicMessage message; + FillRepeated(message); + ExpectRepeated(message); + + // The element type is the ROS value, not the Timestamp backing it. + std::vector<::ros::Time> collected; + for (::ros::Time value : message.stamps) { + collected.push_back(value); + } + EXPECT_EQ(collected, + (std::vector<::ros::Time>{::ros::Time(1, 2), ::ros::Time(3, 4)})); + EXPECT_EQ(ReadSecondsByValue(message.stamps[1]), 3u); +} + +TEST(RosIntrinsicsTest, RepeatedIntrinsicElementsAreAssignable) { + RosIntrinsicMessage message; + FillRepeated(message); + + message.stamps[0] = ::ros::Time(100, 200); + message.fixed_timeouts[1] = ::ros::Duration(-300, 400); + + EXPECT_EQ(message.stamps.Get(0), ::ros::Time(100, 200)); + EXPECT_EQ(message.stamps.Get(1), ::ros::Time(3, 4)); + EXPECT_EQ(message.fixed_timeouts.Get(1), ::ros::Duration(-300, 400)); +} + +TEST(RosIntrinsicsTest, RepeatedIntrinsicsSurviveProtobufWireRoundtrip) { + RosIntrinsicMessage message; + FillRepeated(message); + + RosIntrinsicMessage parsed; + ASSERT_TRUE(parsed.ParseFromString(message.SerializeAsString())); + ExpectRepeated(parsed); +} + +TEST(RosIntrinsicsTest, RepeatedIntrinsicsSurviveRosWireRoundtrip) { + RosIntrinsicMessage message; + FillRepeated(message); + + std::string ros_wire; + ASSERT_TRUE(message.SerializeToROSString(&ros_wire).ok()); + EXPECT_EQ(message.ROSSerializedSize(), ros_wire.size()); + + RosIntrinsicMessage parsed; + ASSERT_TRUE( + parsed.ParseFromROS(absl::Span(ros_wire.data(), + ros_wire.size())) + .ok()); + ExpectRepeated(parsed); +} + +TEST(RosIntrinsicsTest, RepeatedIntrinsicsUseEightRosBytesPerElement) { + RosIntrinsicMessage empty; + RosIntrinsicMessage populated; + populated.stamps.Add(::ros::Time(1, 2)); + populated.stamps.Add(::ros::Time(3, 4)); + populated.timeouts.Add(::ros::Duration(-5, 6)); + + // A fixed extent is always serialized in full, so only the unbounded fields + // move: sec and nsec, four bytes each, with the sequence length unchanged. + EXPECT_EQ(populated.ROSSerializedSize(), empty.ROSSerializedSize() + 3 * 8); +} + +TEST(RosIntrinsicsTest, RepeatedIntrinsicsClone) { + RosIntrinsicMessage source; + FillRepeated(source); + + RosIntrinsicMessage copy(source); + ExpectRepeated(copy); +} + } // namespace } // namespace foo::bar::phaser diff --git a/phaser/ros_metadata_protobuf_frontend_test.cc b/phaser/ros_metadata_protobuf_frontend_test.cc index 9fe2a29..265d40b 100644 --- a/phaser/ros_metadata_protobuf_frontend_test.cc +++ b/phaser/ros_metadata_protobuf_frontend_test.cc @@ -24,6 +24,7 @@ TEST(RosMetadataProtobufFrontendTest, MatchesRosMetadata) { EXPECT_EQ(Wrapper::RosDefinition(), "uint8 READY=1\nexample_msgs/Bool child\nint32[3] samples\n" "example_msgs/Status status\nbool ready\ntime stamp\n" + "time[] stamps\n" "==================================================================" "==============\n" "MSG: example_msgs/Bool\n" @@ -37,7 +38,8 @@ TEST(RosMetadataProtobufFrontendTest, MatchesRosMetadata) { ::phaser::Md5("uint8 READY=1\n" "8b94c1b53db61fb6aed406028ad6332a child\n" "int32[3] samples\n" + - Status::RosMd5() + " status\nbool ready\ntime stamp")); + Status::RosMd5() + + " status\nbool ready\ntime stamp\ntime[] stamps")); } } // namespace diff --git a/phaser/ros_metadata_ros_frontend_test.cc b/phaser/ros_metadata_ros_frontend_test.cc index 27b5549..0e28e85 100644 --- a/phaser/ros_metadata_ros_frontend_test.cc +++ b/phaser/ros_metadata_ros_frontend_test.cc @@ -26,7 +26,8 @@ TEST(RosMetadataRosFrontendTest, MatchesRosMetadata) { ::phaser::Md5("uint8 READY=1\n" "8b94c1b53db61fb6aed406028ad6332a child\n" "int32[3] samples\n" + - Status::RosMd5() + " status\nbool ready\ntime stamp")); + Status::RosMd5() + + " status\nbool ready\ntime stamp\ntime[] stamps")); } } // namespace diff --git a/phaser/ros_wire_conversion_test.cc b/phaser/ros_wire_conversion_test.cc index 7ff9df0..4ecfb95 100644 --- a/phaser/ros_wire_conversion_test.cc +++ b/phaser/ros_wire_conversion_test.cc @@ -189,6 +189,25 @@ std::string ExpectedIntrinsicBytes() { AppendString(bytes, ""); // child label } AppendIntegral(bytes, static_cast(0)); // choice unset + + // Repeated ROS intrinsics: a sequence length only when the extent is not + // fixed, then sec and nsec per element, exactly as a singular one. + AppendIntegral(bytes, static_cast(2)); // stamps count + AppendIntegral(bytes, static_cast(1)); + AppendIntegral(bytes, static_cast(2)); + AppendIntegral(bytes, static_cast(3)); + AppendIntegral(bytes, static_cast(4)); + AppendIntegral(bytes, static_cast(1)); // timeouts count + AppendIntegral(bytes, static_cast(-5)); + AppendIntegral(bytes, static_cast(6)); + AppendIntegral(bytes, static_cast(7)); // fixed_stamps[0] + AppendIntegral(bytes, static_cast(8)); + AppendIntegral(bytes, static_cast(9)); // fixed_stamps[1] + AppendIntegral(bytes, static_cast(10)); + AppendIntegral(bytes, static_cast(-11)); // fixed_timeouts[0] + AppendIntegral(bytes, static_cast(12)); + AppendIntegral(bytes, static_cast(13)); // fixed_timeouts[1] + AppendIntegral(bytes, static_cast(14)); return bytes; } @@ -359,6 +378,13 @@ TEST(ROSWireConversionTest, ROS1IntrinsicsUseNativeLayoutsAndFlushCaches) { message.header->seq = 9; message.header->stamp = ::ros::Time(21, 654); message.header->frame_id = "map"; + message.stamps.Add(::ros::Time(1, 2)); + message.stamps.Add(::ros::Time(3, 4)); + message.timeouts.Add(::ros::Duration(-5, 6)); + message.fixed_stamps.Set(0, ::ros::Time(7, 8)); + message.fixed_stamps.Set(1, ::ros::Time(9, 10)); + message.fixed_timeouts.Set(0, ::ros::Duration(-11, 12)); + message.fixed_timeouts.Set(1, ::ros::Duration(13, 14)); const std::string expected = ExpectedIntrinsicBytes(); ::phaser::ROSBuffer live_output; @@ -457,6 +483,13 @@ TEST(ROSWireConversionTest, ParsedROSPayloadUsesEitherFrontend) { EXPECT_EQ(ros_message.header->stamp.nsec, 654u); EXPECT_EQ(ros_message.header->frame_id, "map"); EXPECT_EQ(ros_message.choice.index(), std::variant_npos); + ASSERT_EQ(ros_message.stamps.size(), 2u); + EXPECT_EQ(ros_message.stamps.Get(0), ::ros::Time(1, 2)); + EXPECT_EQ(ros_message.stamps.Get(1), ::ros::Time(3, 4)); + ASSERT_EQ(ros_message.timeouts.size(), 1u); + EXPECT_EQ(ros_message.timeouts.Get(0), ::ros::Duration(-5, 6)); + EXPECT_EQ(ros_message.fixed_stamps.Get(1), ::ros::Time(9, 10)); + EXPECT_EQ(ros_message.fixed_timeouts.Get(0), ::ros::Duration(-11, 12)); const size_t native_size = ros_message.Size(); std::vector native_payload(native_size); @@ -473,6 +506,15 @@ TEST(ROSWireConversionTest, ParsedROSPayloadUsesEitherFrontend) { EXPECT_FALSE(protobuf_view.has_choice_number()); EXPECT_FALSE(protobuf_view.has_choice_text()); EXPECT_FALSE(protobuf_view.has_choice_child()); + // The ROS frontend wrote these as ROS values; the protobuf frontend reads + // the same payload back as ordinary repeated Timestamp and Duration. + ASSERT_EQ(protobuf_view.stamps_size(), 2); + EXPECT_EQ(protobuf_view.stamps(1).seconds(), 3); + EXPECT_EQ(protobuf_view.stamps(1).nanos(), 4); + ASSERT_EQ(protobuf_view.timeouts_size(), 1); + EXPECT_EQ(protobuf_view.timeouts(0).seconds(), -5); + EXPECT_EQ(protobuf_view.fixed_stamps(0).seconds(), 7); + EXPECT_EQ(protobuf_view.fixed_timeouts(1).nanos(), 14); ProtobufFrontendIntrinsicMessage parsed_protobuf_frontend; ASSERT_TRUE(parsed_protobuf_frontend diff --git a/phaser/runtime/ros.h b/phaser/runtime/ros.h index 506d3cc..b7cfe82 100644 --- a/phaser/runtime/ros.h +++ b/phaser/runtime/ros.h @@ -11,13 +11,18 @@ #include #include +#include #include +#include #include #include #include #include +#include +#include "phaser/runtime/arrays.h" #include "phaser/runtime/fields.h" +#include "phaser/runtime/vectors.h" namespace phaser { @@ -247,6 +252,210 @@ template using RosDurationField = RosMessageField; +// Presents a repeated Phaser message field as a sequence of ROS values, so a +// ROS `time[]` or `duration[]` reads and writes as `::ros::Time` rather than as +// the google.protobuf.Timestamp backing it. `Sequence` is +// MessageVectorField for an unbounded field or +// MessageArrayField for a fixed extent. +// +// Unlike the singular RosMessageField there is no cache: every element access +// converts through `Traits` against the payload, so a mutation is visible to +// the backing message immediately and SyncToPayload has nothing to reconcile. +template +class RosRepeatedMessageField : public Sequence { + public: + using RosType = typename Traits::RosType; + using Sequence::Sequence; + + using value_type = RosType; + using reference = RosType; + using const_reference = RosType; + using size_type = size_t; + using difference_type = ptrdiff_t; + + // Lets `field[i] = value` work without handing out a reference into a + // payload the element does not own. + class Proxy { + public: + Proxy(RosRepeatedMessageField* field, size_t index) + : field_(field), index_(index) {} + + operator RosType() const { return field_->Get(index_); } + RosType Get() const { return field_->Get(index_); } + + Proxy& operator=(const RosType& value) { + field_->Set(index_, value); + return *this; + } + Proxy& operator=(const Proxy& other) { + if (this != &other) { + field_->Set(index_, other.Get()); + } + return *this; + } + + friend bool operator==(const Proxy& lhs, const RosType& rhs) { + return lhs.Get() == rhs; + } + friend bool operator==(const RosType& lhs, const Proxy& rhs) { + return lhs == rhs.Get(); + } + friend bool operator!=(const Proxy& lhs, const RosType& rhs) { + return !(lhs == rhs); + } + friend bool operator!=(const RosType& lhs, const Proxy& rhs) { + return !(lhs == rhs); + } + friend std::ostream& operator<<(std::ostream& os, const Proxy& proxy) { + Traits::Print(os, proxy.Get()); + return os; + } + + private: + RosRepeatedMessageField* field_; + size_t index_; + }; + + class const_iterator { + public: + using iterator_category = std::bidirectional_iterator_tag; + using value_type = RosType; + using difference_type = ptrdiff_t; + using pointer = void; + using reference = RosType; + + const_iterator() = default; + const_iterator(const RosRepeatedMessageField* field, size_t index) + : field_(field), index_(index) {} + + RosType operator*() const { return field_->Get(index_); } + const_iterator& operator++() { + ++index_; + return *this; + } + const_iterator operator++(int) { + const_iterator result = *this; + ++*this; + return result; + } + const_iterator& operator--() { + --index_; + return *this; + } + const_iterator operator--(int) { + const_iterator result = *this; + --*this; + return result; + } + bool operator==(const const_iterator& other) const { + return field_ == other.field_ && index_ == other.index_; + } + bool operator!=(const const_iterator& other) const { + return !(*this == other); + } + + private: + const RosRepeatedMessageField* field_ = nullptr; + size_t index_ = 0; + }; + using iterator = const_iterator; + using reverse_iterator = std::reverse_iterator; + using const_reverse_iterator = std::reverse_iterator; + + const_iterator begin() const { return const_iterator(this, 0); } + const_iterator end() const { return const_iterator(this, this->size()); } + const_iterator cbegin() const { return begin(); } + const_iterator cend() const { return end(); } + const_reverse_iterator rbegin() const { + return const_reverse_iterator(end()); + } + const_reverse_iterator rend() const { + return const_reverse_iterator(begin()); + } + const_reverse_iterator crbegin() const { return rbegin(); } + const_reverse_iterator crend() const { return rend(); } + + // An out-of-range index needs no guard here: the sequence hands back a + // default-constructed backend for one, which loads as a zero ROS value. + RosType Get(size_t index) const { + RosType value; + Traits::Load(Sequence::Get(index), value); + return value; + } + + void Set(size_t index, const RosType& value) { + auto backend = Sequence::Mutable(index); + Traits::Store(value, backend); + } + + void Add(const RosType& value) { + auto backend = Sequence::Add(); + Traits::Store(value, backend); + } + + void push_back(const RosType& value) { Add(value); } + + RosType operator[](size_t index) const { return Get(index); } + Proxy operator[](size_t index) { return Proxy(this, index); } + + RosType front() const { return Get(0); } + Proxy front() { return Proxy(this, 0); } + RosType back() const { return Get(this->size() - 1); } + Proxy back() { return Proxy(this, this->size() - 1); } + + std::vector Get() const { + std::vector result; + result.reserve(this->size()); + for (size_t i = 0; i < this->size(); ++i) { + result.push_back(Get(i)); + } + return result; + } + + // Formats one element the way the singular field formats its value. Without + // this a repeated element would stream through ROS's own operator<<, which + // prints a different shape than the rest of the message. + static std::ostream& PrintElement(std::ostream& os, const RosType& value) { + Traits::Print(os, value); + return os; + } + + bool operator==(const RosRepeatedMessageField& other) const { + if (this->size() != other.size()) { + return false; + } + for (size_t i = 0; i < this->size(); ++i) { + if (!(Get(i) == other.Get(i))) { + return false; + } + } + return true; + } + bool operator!=(const RosRepeatedMessageField& other) const { + return !(*this == other); + } +}; + +template +using RosTimeVectorField = + RosRepeatedMessageField, + internal::RosTimeTraits>; + +template +using RosDurationVectorField = + RosRepeatedMessageField, + internal::RosDurationTraits>; + +template +using RosTimeArrayField = + RosRepeatedMessageField, + internal::RosTimeTraits>; + +template +using RosDurationArrayField = + RosRepeatedMessageField, + internal::RosDurationTraits>; + template class RosHeaderMutableView { public: diff --git a/phaser/self_named_field_test.cc b/phaser/self_named_field_test.cc new file mode 100644 index 0000000..9bfab65 --- /dev/null +++ b/phaser/self_named_field_test.cc @@ -0,0 +1,99 @@ +#include + +#include "gtest/gtest.h" +#include "phaser/testdata/SelfNamedField.phaser.h" +#include "phaser/testdata/self_named_field_ros_phaser/phaser/testdata/SelfNamedField.phaser.h" + +namespace selfnamed { +namespace { + +// The protobuf frontend normally puts the accessor at the field name and the +// member one underscore past it. A self-named field pushes both along by one so +// the accessor stops being a constructor. +TEST(SelfNamedFieldTest, ProtobufFrontendShiftsTheAccessorPastTheClassName) { + phaser::Polygon polygon; + phaser::Point point = polygon.add_Polygon(); + point.set_x(1.5); + point.set_y(-2.5); + + ASSERT_EQ(1u, polygon.Polygon_size()); + EXPECT_EQ(1.5, polygon.Polygon_(0).x()); + EXPECT_EQ(-2.5, polygon.Polygon_(0).y()); +} + +TEST(SelfNamedFieldTest, ProtobufFrontendHandlesScalarSelfNamedField) { + phaser::Label label; + label.set_Label("stop"); + label.set_index(3); + EXPECT_EQ("stop", label.Label_()); + EXPECT_EQ(3, label.index()); +} + +// The ROS frontend exposes the member itself, so only the member has to move. +TEST(SelfNamedFieldTest, RosFrontendSuffixesTheCollidingMember) { + phaser_ros::Polygon polygon; + auto point = polygon.Polygon_.Add(); + point.x = 4.0; + point.y = 5.0; + + ASSERT_EQ(1u, polygon.Polygon_.size()); + EXPECT_EQ(4.0, polygon.Polygon_[0].x); + EXPECT_EQ(5.0, polygon.Polygon_[0].y); +} + +TEST(SelfNamedFieldTest, RosFrontendLeavesOtherMembersAlone) { + phaser_ros::Label label; + label.Label_ = "go"; + label.index = 7; + EXPECT_EQ("go", label.Label_.Get()); + EXPECT_EQ(7, label.index); +} + +// Nested messages generate as `Outer_Inner`, so that is the name a member has +// to stay clear of, not the proto-level `Inner`. +TEST(SelfNamedFieldTest, RosFrontendUsesTheFlattenedNestedClassName) { + phaser_ros::Outer_Inner inner; + inner.Outer_Inner_ = 11; + inner.keep = 12; + EXPECT_EQ(11, inner.Outer_Inner_); + EXPECT_EQ(12, inner.keep); +} + +TEST(SelfNamedFieldTest, SelfNamedFieldSurvivesAWireRoundtrip) { + phaser_ros::Label label; + label.Label_ = "yield"; + label.index = 2; + + const std::string wire = label.SerializeAsString(); + + phaser::Label parsed; + ASSERT_TRUE(parsed.ParseFromString(wire)); + EXPECT_EQ("yield", parsed.Label_()); + EXPECT_EQ(2, parsed.index()); +} + +TEST(SelfNamedFieldTest, FieldNamedTDoesNotCollideWithCloneFromsTemplate) { + phaser_ros::Horizon source; + source.T = 20; + source.steps.push_back(0.5); + + phaser_ros::Horizon copy; + ASSERT_TRUE(copy.CloneFrom(source).ok()); + EXPECT_EQ(20, copy.T); + ASSERT_EQ(1u, copy.steps.size()); + EXPECT_EQ(0.5, copy.steps[0]); +} + +TEST(SelfNamedFieldTest, CloneFromCopiesASelfNamedField) { + phaser::Label source; + source.set_Label("merge"); + source.set_index(5); + + phaser::Label copy; + ASSERT_TRUE(copy.CloneFrom(source).ok()); + EXPECT_EQ("merge", copy.Label_()); + EXPECT_EQ(5, copy.index()); +} + +} // namespace +} // namespace selfnamed diff --git a/phaser/testdata/AliasedEnum.proto b/phaser/testdata/AliasedEnum.proto new file mode 100644 index 0000000..9eef8c3 --- /dev/null +++ b/phaser/testdata/AliasedEnum.proto @@ -0,0 +1,21 @@ +syntax = "proto3"; + +package alias; + +// `allow_alias` lets several names share one number. Protobuf accepts it, so +// the generated stringizer has to pick one name per number rather than emit a +// switch case for each. +enum Signal { + option allow_alias = true; + + SIGNAL_UNSPECIFIED = 0; + SIGNAL_IDLE = 0; + SIGNAL_BUSY = 1; + SIGNAL_WORKING = 1; + SIGNAL_DONE = 2; +} + +message Job { + Signal signal = 1; + repeated Signal history = 2; +} diff --git a/phaser/testdata/BUILD b/phaser/testdata/BUILD index adc59dd..0ead64a 100644 --- a/phaser/testdata/BUILD +++ b/phaser/testdata/BUILD @@ -223,9 +223,12 @@ sh_test( "$(rootpath @com_google_protobuf//:protoc)", "$(rootpath //phaser/compiler:phaser)", "$(rootpath InvalidRosIntrinsic.proto)", + "$(rootpath InvalidRosIntrinsicOneof.proto)", ], data = [ "InvalidRosIntrinsic.proto", + "InvalidRosIntrinsicOneof.proto", + "RosHeader.proto", "//phaser:valgrind.supp", "//phaser/compiler:phaser", "@com_google_protobuf//:protoc", @@ -233,6 +236,72 @@ sh_test( ], ) +proto_library( + name = "self_named_field_proto", + srcs = ["SelfNamedField.proto"], +) + +phaser_library( + name = "self_named_field_phaser", + add_namespace = "phaser", + runtime = "//phaser/runtime:phaser_runtime", + deps = [":self_named_field_proto"], +) + +phaser_library( + name = "self_named_field_ros_phaser", + add_namespace = "phaser_ros", + cc_deps = [":ros1_shim"], + direct_header_symlinks = False, + frontend = "ros", + runtime = "//phaser/runtime:phaser_runtime", + deps = [":self_named_field_proto"], +) + +proto_library( + name = "aliased_enum_proto", + srcs = ["AliasedEnum.proto"], +) + +phaser_library( + name = "aliased_enum_phaser", + add_namespace = "phaser", + runtime = "//phaser/runtime:phaser_runtime", + deps = [":aliased_enum_proto"], +) + +proto_library( + name = "cross_package_enum_proto", + srcs = ["CrossPackageEnum.proto"], + deps = ["//phaser:options_proto"], +) + +proto_library( + name = "cross_package_enum_user_proto", + srcs = ["CrossPackageEnumUser.proto"], + deps = [ + ":cross_package_enum_proto", + "//phaser:options_proto", + ], +) + +phaser_library( + name = "cross_package_enum_phaser", + add_namespace = "phaser", + runtime = "//phaser/runtime:phaser_runtime", + deps = [":cross_package_enum_user_proto"], +) + +phaser_library( + name = "cross_package_enum_ros_phaser", + add_namespace = "phaser_ros", + cc_deps = [":ros1_shim"], + direct_header_symlinks = False, + frontend = "ros", + runtime = "//phaser/runtime:phaser_runtime", + deps = [":cross_package_enum_user_proto"], +) + proto_library( name = "coverage_proto", srcs = ["coverage.proto"], @@ -272,4 +341,4 @@ phaser_library( add_namespace = "phaser", runtime = "//phaser/runtime:phaser_runtime", deps = [":vision_proto"], -) +) \ No newline at end of file diff --git a/phaser/testdata/CrossPackageEnum.proto b/phaser/testdata/CrossPackageEnum.proto new file mode 100644 index 0000000..15c1336 --- /dev/null +++ b/phaser/testdata/CrossPackageEnum.proto @@ -0,0 +1,27 @@ +syntax = "proto3"; + +package palette; + +import "phaser/options.proto"; + +enum Color { + COLOR_RED = 0; + COLOR_GREEN = 1; + COLOR_BLUE = 2; +} + +message Shade { + enum Depth { + DEPTH_LIGHT = 0; + DEPTH_DARK = 1; + } + + Depth depth = 1; +} + +message Palette { + Color primary = 1; + repeated Color swatches = 2; + repeated Color triad = 3 [(phaser.array_size) = 3]; + Shade.Depth depth = 4; +} diff --git a/phaser/testdata/CrossPackageEnumUser.proto b/phaser/testdata/CrossPackageEnumUser.proto new file mode 100644 index 0000000..0cef233 --- /dev/null +++ b/phaser/testdata/CrossPackageEnumUser.proto @@ -0,0 +1,22 @@ +syntax = "proto3"; + +package canvas; + +import "phaser/options.proto"; +import "phaser/testdata/CrossPackageEnum.proto"; + +// Every enum field here refers to an enum declared in another proto package, so +// the generated code has to name the enum, its stringizer, and its parser with +// that package's namespace rather than the short name. +message Drawing { + palette.Color background = 1; + repeated palette.Color strokes = 2; + repeated palette.Color corners = 3 [(phaser.array_size) = 4]; + palette.Shade.Depth depth = 4; + palette.Palette palette = 5; + + oneof accent { + palette.Color accent_color = 6; + int32 accent_index = 7; + } +} diff --git a/phaser/testdata/InvalidRosIntrinsic.proto b/phaser/testdata/InvalidRosIntrinsic.proto index a44740c..66050ba 100644 --- a/phaser/testdata/InvalidRosIntrinsic.proto +++ b/phaser/testdata/InvalidRosIntrinsic.proto @@ -2,8 +2,8 @@ syntax = "proto3"; package invalid; -import "google/protobuf/timestamp.proto"; +import "phaser/testdata/RosHeader.proto"; message InvalidRosIntrinsic { - repeated google.protobuf.Timestamp stamps = 1; + repeated std_msgs.Header headers = 1; } diff --git a/phaser/testdata/InvalidRosIntrinsicOneof.proto b/phaser/testdata/InvalidRosIntrinsicOneof.proto new file mode 100644 index 0000000..b1cf4a8 --- /dev/null +++ b/phaser/testdata/InvalidRosIntrinsicOneof.proto @@ -0,0 +1,12 @@ +syntax = "proto3"; + +package invalid; + +import "google/protobuf/timestamp.proto"; + +message InvalidRosIntrinsicOneof { + oneof choice { + google.protobuf.Timestamp stamp = 1; + int32 count = 2; + } +} diff --git a/phaser/testdata/RosIntrinsics.proto b/phaser/testdata/RosIntrinsics.proto index 5ef6d44..154d5d4 100644 --- a/phaser/testdata/RosIntrinsics.proto +++ b/phaser/testdata/RosIntrinsics.proto @@ -29,4 +29,11 @@ message RosIntrinsicMessage { string choice_text = 12; WireChild choice_child = 13; } + + repeated google.protobuf.Timestamp stamps = 14; + repeated google.protobuf.Duration timeouts = 15; + repeated google.protobuf.Timestamp fixed_stamps = 16 + [(phaser.array_size) = 2]; + repeated google.protobuf.Duration fixed_timeouts = 17 + [(phaser.array_size) = 2]; } diff --git a/phaser/testdata/RosIntrinsicsProtobufFrontend.proto b/phaser/testdata/RosIntrinsicsProtobufFrontend.proto index a2dd101..76a7693 100644 --- a/phaser/testdata/RosIntrinsicsProtobufFrontend.proto +++ b/phaser/testdata/RosIntrinsicsProtobufFrontend.proto @@ -29,4 +29,11 @@ message RosIntrinsicMessage { string choice_text = 12; WireChild choice_child = 13; } + + repeated google.protobuf.Timestamp stamps = 14; + repeated google.protobuf.Duration timeouts = 15; + repeated google.protobuf.Timestamp fixed_stamps = 16 + [(phaser.array_size) = 2]; + repeated google.protobuf.Duration fixed_timeouts = 17 + [(phaser.array_size) = 2]; } diff --git a/phaser/testdata/RosMetadata.proto b/phaser/testdata/RosMetadata.proto index cc62ec2..37edbeb 100644 --- a/phaser/testdata/RosMetadata.proto +++ b/phaser/testdata/RosMetadata.proto @@ -37,4 +37,5 @@ message Wrapper { Status status = 3; bool ready = 4; google.protobuf.Timestamp stamp = 5; + repeated google.protobuf.Timestamp stamps = 6; } diff --git a/phaser/testdata/SelfNamedField.proto b/phaser/testdata/SelfNamedField.proto new file mode 100644 index 0000000..4aaa638 --- /dev/null +++ b/phaser/testdata/SelfNamedField.proto @@ -0,0 +1,35 @@ +syntax = "proto3"; + +package selfnamed; + +message Point { + double x = 1; + double y = 2; +} + +// A ROS message is free to give a field the same name as the message itself, +// which C++ will not accept as a member of the generated class. +message Polygon { + repeated Point Polygon = 1; +} + +message Label { + string Label = 1; + int32 index = 2; +} + +message Outer { + message Inner { + int32 Outer_Inner = 1; + int32 keep = 2; + } + + Inner inner = 1; +} + +// `T` is a plausible .msg field name and also the obvious name for a template +// parameter on the generated CloneFrom. +message Horizon { + int32 T = 1; + repeated double steps = 2; +} diff --git a/phaser/testdata/invalid_ros_intrinsic_test.sh b/phaser/testdata/invalid_ros_intrinsic_test.sh index bf2a8e1..d771e85 100755 --- a/phaser/testdata/invalid_ros_intrinsic_test.sh +++ b/phaser/testdata/invalid_ros_intrinsic_test.sh @@ -3,21 +3,34 @@ set -euo pipefail protoc="$1" plugin="$2" -invalid_proto="$3" +repeated_header_proto="$3" +oneof_proto="$4" out_dir="$(mktemp -d)" trap 'rm -rf "${out_dir}"' EXIT root="${TEST_SRCDIR}/${TEST_WORKSPACE}" proto_include="${TEST_SRCDIR}/protobuf+/src" -if ! "${protoc}" \ - --plugin="protoc-gen-phaser=${plugin}" \ - -I"${root}" \ - -I"${proto_include}" \ - --phaser_out="frontend=ros,add_namespace=phaser:${out_dir}" \ - "${invalid_proto}" 2>"${out_dir}/err.txt"; then - grep -q "must be singular and cannot be in a oneof" "${out_dir}/err.txt" - exit 0 -fi -echo "expected phaser plugin to reject repeated ROS intrinsic" >&2 -exit 1 +# A repeated time or duration is supported; these two remain rejected. +check_rejected() { + local proto="$1" + local expected="$2" + local err="${out_dir}/err.txt" + if "${protoc}" \ + --plugin="protoc-gen-phaser=${plugin}" \ + -I"${root}" \ + -I"${proto_include}" \ + --phaser_out="frontend=ros,add_namespace=phaser:${out_dir}" \ + "${proto}" 2>"${err}"; then + echo "expected phaser plugin to reject ${proto}" >&2 + exit 1 + fi + if ! grep -q "${expected}" "${err}"; then + echo "expected ${proto} to be rejected with '${expected}', got:" >&2 + cat "${err}" >&2 + exit 1 + fi +} + +check_rejected "${repeated_header_proto}" "does not support a repeated Header" +check_rejected "${oneof_proto}" "cannot be in a oneof"