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

Return NULL for empty ParametersNode #962

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

haldun
Copy link
Collaborator

@haldun haldun commented Jun 8, 2023

Fixes #961

@kddnewton
Copy link
Collaborator

A couple of thoughts on this.

  1. I think the case where we have invalid parameters should still add parameters to the list. For example def foo(A), you'd still expect there to be a node in the list of required parameters, even though it's incorrect. That way there's a place to attach the error message and formatters/linters have something to point to.
  2. Instead of allocating the parameters node and then immediately deallocating, I would much prefer that we simply never allocate in the first place. I think this is probably going to be solved by the first point above, but there may be other cases where we need to look forward instead.

@haldun
Copy link
Collaborator Author

haldun commented Jun 8, 2023

@kddnewton About your first point, is c1fe0ec close to what you suggest?

About 2, I think I don't know how to do it otherwise. Don't we need a place in memory to collect the parameters?

@kddnewton
Copy link
Collaborator

@haldun I've come around on this. Could you rebase and make sure the tests pass?

@haldun
Copy link
Collaborator Author

haldun commented Jul 2, 2023

@kddnewton Done in f82ef87

@jemmaissroff jemmaissroff merged commit 3e3bf1c into ruby:main Jul 10, 2023
flavorjones added a commit to flavorjones/yarp that referenced this pull request Jul 18, 2023
- ignore lib/yarp.bundle
- :check_manifest task is a dependency of :test
- remove yp_snprintf.c from the gemspec (see 4793b1c / ruby#962)
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.

Fix cases where we create a ParamtersNode with no parameters
3 participants