Support repeated ROS intrinsics, cross-package enums, aliased enums, and self-named fields - #7
Merged
Merged
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four cases the generator could not express. Each is independent; they are together because they share test scaffolding.
Repeated
timeanddurationThe ROS frontend rejected any repeated ROS intrinsic:
Because the frontend compiles the whole transitive proto set, a single
time[]field blocked every schema that transitively depended on it, not just its own.RosRepeatedMessageField(inphaser/runtime/ros.h) wrapsMessageVectorField<Backend>orMessageArrayField<Backend, N>and converts per element, so atime[]field reads and writes as::ros::Timerather than thegoogle.protobuf.Timestampbacking it. Unlike the singularRosMessageFieldthere is no cache: every access converts against the payload, so a mutation is visible to the backing message immediately andSyncToPayloadhas nothing to reconcile. AProxyreturn from the mutableoperator[]letsfield[i] = valuework without handing out a reference into a payload the element does not own.Two rejections remain, and the error messages now say which applies:
Header.RosHeaderFieldhands out a view whose lifetime is tied to the field rather than a value, so it has no element type to repeat.oneof.Cross-package enums
Enums, along with their stringizer and parser, are emitted at namespace scope, so the short name only resolved inside the package that declared them. A field referencing an enum from another proto package generated an unqualified name that did not compile.
EnumNamenow spells out the declaring package's namespace and the added namespace, mirroring whatMessageNamealready did.Aliased enums
option allow_aliaslets several names share one number. The stringizer emitted onecaseper name, so an aliased enum produced aswitchwith duplicate case labels:Only the first name declared for a number gets a case now, which is also what protobuf's own
_Namereports for that number.Fields named after their own message
A
.msgfile may name a field after the message holding it, which C++ reads as a constructor declaration rather than a member:Such a field now gets trailing underscores, as reserved words already did.
protocnever meets this because it lowercases accessors, and roscpp never meets it because it names the structFoo_<Allocator>and typedefsFooto that. The two frontends differ: the ROS frontend exposes the member directly so only the member moves, while the protobuf frontend keeps the accessor at the field name and pushes the member one underscore past it. Prefixed accessors such asset_xandadd_xare already legal whatever the field is called, so only the bare accessor is renamed.CloneFrom's template parameter is also renamedT->_phaser_Source, sinceTis a plausible field name and would win name lookup inside the template.Tests
Six new test files plus extensions to existing ones, all under
bazel test //...(22 targets, all passing):ros_intrinsics_test.cc— repeated intrinsics: API, assignability, protobuf and ROS wire roundtrips, 8-bytes-per-element ROS layout,CloneFrom.ros_wire_conversion_test.cc— repeated intrinsics added to the golden ROS byte stream and to the either-frontend parse test.cross_package_enum_test.cc/cross_package_enum_ros_test.cc— singular, repeated, sized-array, nested, and union enum fields across packages, on both frontends.aliased_enum_test.cc— aliases compare equal, stringizer reports the first name, parser accepts every alias.self_named_field_test.cc— self-named fields on both frontends, the flattened nested class name, wire roundtrip, andCloneFrom.invalid_ros_intrinsic_test.sh— now checks both remaining rejections separately (repeatedHeader, and an intrinsic in aoneofvia the newInvalidRosIntrinsicOneof.proto).RosMetadata.protogains arepeated Timestampfield so the ROS definition and MD5 expectations covertime[].