Skip to content
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.

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

Adding support for 2D string arrays similar to 2D double arrays. #407

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jasonmreding
Copy link
Collaborator

Description

This PR is a follow on to #404 and adds built in support for 2D arrays of strings based on array.proto. With this change, authors of grpc services and clients of the service can utilize the built in type for 2D arrays of strings on the diagram without having to convert to/from the equivalent grpc message type that gets sent across the wire. Instead, the conversion is handled internally for the user.

Implementation / Design

C++:

  • Split wellknown::Types enum and well known message classes into separate files. This avoided some circular dependencies in the header files and eliminates the need to forward declare types instead of including the header directly to break the cycle.
  • Created base class and interface class for 2D message classes to better single source common code. Most of the static methods are now virtual functions, and there is a singleton GetInstance method which is used instead of static methods.
  • Introduced String2DArray message class.
  • Most everything else is just following usages of wellknown::Types enum and updating the code as appropriate.

LV:

  • Added new String2DArray enum value to Well Known Types.ctl
  • Most everything else was following usages of the enum type and updating accordingly and/or fixing broken case structures that no longer had a case for every enum value. To make it easier to find spots in the code that need to be revisited when adding a new well known type, I removed default case structure cases that returned a runtime error. This means you can utilize compile time errors rather than runtime errors when fixing up the code.

Testing

I manually tested code generation of client and server and round tripping of data with the following proto file.

syntax = "proto3";

package echo.array;

import "ni/protobuf/types/array.proto";

service EchoArray {
  rpc Echo2DArray(Echo2DArrayRequest) returns(Echo2DArrayResponse);
  rpc EchoRepeated2DArray(EchoRepeated2DArrayRequest) returns(EchoRepeated2DArrayResponse);
}

message NestedMessage {
  oneof union
  {
    string string_value = 1 ;
    ni.protobuf.types.Double2DArray double_2d_array = 2;
    NestedMessage2 nested_message_2 = 3;
    ni.protobuf.types.String2DArray string_2d_array = 4;
  }
}

message NestedMessage2 {
  int32 int_value = 1;
}

message Echo2DArrayRequest {
  bool bool_value = 1 ;
  ni.protobuf.types.Double2DArray array1 = 2;
  NestedMessage nested_message = 3;
  ni.protobuf.types.Double2DArray array2 = 4;
  ni.protobuf.types.String2DArray string_array = 5;
}

message Echo2DArrayResponse {
  bool bool_value = 1;
  ni.protobuf.types.Double2DArray array1 = 2;
  NestedMessage nested_message = 3;
  ni.protobuf.types.Double2DArray array2 = 4;
  ni.protobuf.types.String2DArray string_array = 5;
}

message EchoRepeated2DArrayRequest {
  repeated ni.protobuf.types.Double2DArray repeated_double_array = 1;
  repeated ni.protobuf.types.String2DArray repeated_string_array = 2;
}

message EchoRepeated2DArrayResponse {
  repeated ni.protobuf.types.Double2DArray repeated_double_array = 1;
  repeated ni.protobuf.types.String2DArray repeated_string_array = 2;
}

I also verified the following cluster packs/unpacks correctly.

image

Reviewers

  • Adding @bkeryan to focus on feedback of C++ changes.
  • Adding @dixonjoel to focus on feedback of LV changes.
  • Everyone else is FYI

src/well_known_messages.cc Outdated Show resolved Hide resolved
src/well_known_messages.cc Outdated Show resolved Hide resolved
src/well_known_messages.cc Outdated Show resolved Hide resolved
- Use reserve/move semantics for repeated strings.
- Use sizeof(LStrHandle) instead of sizeof(char*)
@jasonmreding jasonmreding requested a review from bkeryan February 7, 2025 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants