Skip to content

Conversation

rjhdby
Copy link

@rjhdby rjhdby commented Feb 26, 2019

Correctly fail on #3874

Expressions that are not computed on compile time is excluded (e.g. objects)

@Danack
Copy link
Contributor

Danack commented Feb 27, 2019

I can't follow this test. It's quite important to be able to understand what a test is going to be testing without having to run the code.

Please could you change the PR to be the generated code, with the code to regenerate it as a comment.

@rjhdby
Copy link
Author

rjhdby commented Feb 27, 2019

@Danack
There is about 2209 generated lines of code and they relies on runtime checks results.
Definitely I can do that you say, but maybe it will be enough to make code-generation more clear and describe behavior in comment?

@bwoebi
Copy link
Member

bwoebi commented Feb 27, 2019

@Danack I think this test can be merged as is. It basically ensures that compile time and runtime comparisons behave the same way - i.e. it makes it impossible to change behavior of compile time comparisons without also changing runtime comparisons. I.e. it generates code that checks whether the equality behavior of the input values in the big array respective to each other remains consistent with the operators.

This test is not useful in its generated version: if we ever change any comparison or add an operator, we'll have massive overhead there. And the diff output will also exactly give you a sample of what's wrong. I'll cherry-pick it.

$f = 0;

function prepareLine($op1, $op2, $cmp, $type){
$cmp = (int) $cmp;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why go through the int cast here and not makeParam() this as well?

return "\$c++;if ($cmp !== (int)($op1_p $type $op2_p)){echo \"$op1_p $type $op2_p\n\";\$f++;};\n";
}

$filename = dirname(__FILE__) . DIRECTORY_SEPARATOR . 'compare_equality_temp.php';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: __DIR__

$var_cnt = count($input);

foreach($input as $var) {
for ($i = 0; $i < $var_cnt; $i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foreach ($input as $var2)?

@nikic
Copy link
Member

nikic commented Feb 27, 2019

@rjhdby Maybe add a small comment explaining that this test generates code to compare compile-time evaluation of comparisons with run-time evaluation?

@rjhdby
Copy link
Author

rjhdby commented Feb 27, 2019

@nikic

nit: dirname(FILE) => DIR
At the beginning I used DIR, but late take a look at proc_open_bug_69900.phpt and, just in case, change it identically. I think "Maybe it is critical for run_tests?" :D

Why go through the int cast here and not makeParam() this as well?
I'm afraid I did not understand why?
$cmp contains runtime compare result(bool). I cast it to int in order to disable autocast boot=>string("","1") when generate line of code.
makeParam() returns value of param as string for print it into temp file.

foreach ($input as $var2)?
I just copied loops from other "compare" tests.

Maybe add a small comment explaining that this test generates code
to compare compile-time evaluation of comparisons with run-time evaluation?
Ok

PS Sorry for answering by separate message. My browser is too old for github and I can't update it because of corporate rules

@krakjoe
Copy link
Member

krakjoe commented Feb 27, 2019

As nikita said, a description section would really be a benefit for readers ... I too didn't understand the test, and once it's merged all context from github conversations is lost ...

@cmb69
Copy link
Member

cmb69 commented Feb 27, 2019

It seems this PR has been applied; if so, it should be closed. Anyhow, it would be nice to fix the failing test.

@bwoebi
Copy link
Member

bwoebi commented Feb 27, 2019

I will in an hour - was busy until now

@php-pulls
Copy link

Comment on behalf of bwoebi at php.net:

Implemented via fcfec91 (and a small update to delay notices until runtime in 94d3e40).

@php-pulls php-pulls closed this Feb 27, 2019
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.

7 participants