Skip to content

Ruby: rename max_recursion_depth to recursion_limit#9486

Merged
acozzette merged 1 commit intoprotocolbuffers:masterfrom
acozzette:recursion-limit
Feb 9, 2022
Merged

Ruby: rename max_recursion_depth to recursion_limit#9486
acozzette merged 1 commit intoprotocolbuffers:masterfrom
acozzette:recursion-limit

Conversation

@acozzette
Copy link
Copy Markdown

This will help keep the terminology consistent with the other language
implementations.

This will help keep the terminology consistent with the other language
implementations.
@haberman
Copy link
Copy Markdown
Member

haberman commented Feb 9, 2022

Thanks Adam!

One related thing that would be helpful, if you have the time, is to verify that the recursion limit here has the same semantics as in C++. In other words, it would be nice if there weren't off-by-one errors, where using the same recursion limit in two languages yielded slightly different results.

@acozzette
Copy link
Copy Markdown
Author

Let me see about adding a conformance test, since that seems like it would be the most reliable way to make sure languages are enforcing the same recursion limit.

@acozzette acozzette merged commit d5ef16c into protocolbuffers:master Feb 9, 2022
@acozzette acozzette deleted the recursion-limit branch February 9, 2022 18:21
@aleksandarabas
Copy link
Copy Markdown

NO NO NO, some people will mistake it as a rushed abbreviation for recursion limit case.
But the stack depth limit which is imposed on the recursion process must be clearly distinguished from whatever custom property that doesn't require privilege escalation. This will help avoid ww3. thank you

@aleksandarabas
Copy link
Copy Markdown

Let me see about adding a conformance test, since that seems like it would be the most reliable way to make sure languages are enforcing the same recursion limit.

This is very plainly why you struggle with the phenomena of small government republicans. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants