Treat single splats with same restriction as equivalent #12584
Merged
Conversation
This file contains 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
HertzDevil
added
kind:bug
Comment on lines
+361
to
+372
| def old_min_args_size | ||
| if splat_index = self.def.splat_index | ||
| args = self.def.args | ||
| unless args[splat_index].name.empty? | ||
| default_value_index = args.index(&.default_value) | ||
| min_size = default_value_index || args.size | ||
| min_size -= 1 unless default_value_index.try(&.< splat_index) | ||
| return min_size | ||
| end | ||
| end | ||
| self.min_size | ||
| end |
There was a problem hiding this comment.
This implementation looks very similar to Def#min_max_args_sizes. Could we just use that?
There was a problem hiding this comment.
First the max size is unaffected, second we already know that -Dpreview_overload_order is not in effect at this point. If we check for that flag in min_max_args_sizes itself there could be a performance impact and some other uses of the method also become inconsistent (e.g. too-few-argunents errors).
There might be a better way to derive the old min size from the new min size. I don't know if it would end up doing the same amount of work though.
There was a problem hiding this comment.
I'm okay with leaving it like that. But maybe add a comment about this to the code?
straight-shoota
approved these changes
Oct 10, 2022
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.
Fixes #12579 by ensuring the old behavior of
Crystal::Def#min_max_args_sizesis used during overload ordering. (The new one is still used in other places like error reporting.)Has no effect on
-Dpreview_overload_order.