-
Notifications
You must be signed in to change notification settings - Fork 594
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
fix: detect cycle ref in proto #10499
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
|
||
� | ||
recursive.proto recursive"� | ||
ComplexRecursiveMessage | ||
node_name ( RnodeName | ||
node_id (RnodeIdM | ||
Comment on lines
+2
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the compiled result? Might be better to git ignore it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the proto dep requires the compiled ver There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean can we compile it when testing? Seem we don't have any other test that depends on the hard coded compile output. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind, not a big deal. We can always handle it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There are only a few generated protobuf binary, I think it is not worthy introducing extra scripts and toolchain. |
||
|
||
attributes (2-.recursive.ComplexRecursiveMessage.AttributesR | ||
attributesA | ||
parent (2).recursive.ComplexRecursiveMessage.ParentRparent> | ||
children (2".recursive.ComplexRecursiveMessageRchildren4 | ||
|
||
Attributes | ||
key ( Rkey | ||
value ( Rvalue� | ||
Parent | ||
parent_name ( R | ||
parentName | ||
parent_id (RparentId> | ||
siblings (2".recursive.ComplexRecursiveMessageRsiblingsbproto3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
syntax = "proto3"; | ||
|
||
package recursive; | ||
|
||
message ComplexRecursiveMessage { | ||
string node_name = 1; | ||
int32 node_id = 2; | ||
|
||
message Attributes { | ||
string key = 1; | ||
string value = 2; | ||
} | ||
|
||
repeated Attributes attributes = 3; | ||
|
||
message Parent { | ||
string parent_name = 1; | ||
int32 parent_id = 2; | ||
repeated ComplexRecursiveMessage siblings = 3; | ||
} | ||
|
||
Parent parent = 4; | ||
Comment on lines
+16
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a practical example? Seems we're storing the nodes multiple times since protobuf is by-value instead of by-ref. 😂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the example is generated by chatgpt. real world ones are welcomed |
||
repeated ComplexRecursiveMessage children = 5; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's cool to use an O(N^2) algorithm in this case. But I am worrying if it's possible for users to import a 1k-fields message in the system, which may cause the memory usage suddenly increases.
Given that all the previous types before the current DFS point should be unique, maybe you can use a set instead of a vector:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use a vec displaying the parser visiting path in order to help to identify the recursive ref occurs at which level.
push and pop op always happen at the end of the vec, there should be little overhead for allocation