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

Arguments count should be checked prior individual argument type #15037

Closed
mvorisek opened this issue Jul 20, 2024 · 9 comments
Closed

Arguments count should be checked prior individual argument type #15037

mvorisek opened this issue Jul 20, 2024 · 9 comments

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Jul 20, 2024

from https://bugs.php.net/bug.php?id=81595

Description

If arguments count does not match, it should be reported instead of trying to check the arguments type first.

https://3v4l.org/X53qH

Test script:

function test(int $a, string $b) {
}

try {
    test(1);
} catch (Error $e) { echo $e->getMessage() . "\n"; }

try {
    test('test');
} catch (Error $e) { echo $e->getMessage() . "\n"; }

Expected result:

Too few arguments to function test(), 1 passed in /in/X53qH on line 7 and exactly 2 expected
Too few arguments to function test(), 1 passed in /in/X53qH on line 7 and exactly 2 expected

Actual result:

Too few arguments to function test(), 1 passed in /in/X53qH on line 7 and exactly 2 expected
test(): Argument #1 ($a) must be of type int, string given, called in /in/X53qH on line 11
@cmb69
Copy link
Member

cmb69 commented Jul 20, 2024

Thank you! I have copied over the description from the old bug tracker. @nikic's question remains unanswered:

Why is this order better?

I think I understand your point, but does the order of the checks really matter for userland developers? After all, they need to fix the function call. And for the implementation, the current way is likely the most convenient and fastest.

@iluuu1994
Copy link
Member

Adding another check seems pointless. It would just add a slowdown for no particular reason.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 20, 2024

The slowdown will be very minimal.

@cmb69
Copy link
Member

cmb69 commented Jul 20, 2024

The slowdown will be very minimal.

Is that so?

@devnexen
Copy link
Member

The slowdown will be very minimal.

How do you know ?

@mvorisek
Copy link
Contributor Author

That is my guess. I am not C developer so I cannot verify this easily. Can we run some benchmark?

@devnexen
Copy link
Member

Can we run some benchmark?

Thanks :) ..

@devnexen
Copy link
Member

The way I see it, speaking for myself. Not voting yes for this, I see no benefits and surely not perf improvements.

Copy link

github-actions bot commented Aug 4, 2024

No feedback was provided. The issue is being suspended because we assume that you are no longer experiencing the problem. If this is not the case and you are able to provide the information that was requested earlier, please do so. Thank you.

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

No branches or pull requests

4 participants