Skip to content

zend_hash_sort never fails since PHP 5.5.0#20

Closed
Jan-E wants to merge 1 commit into
php:masterfrom
Jan-E:zend-hash-sort-never-fails
Closed

zend_hash_sort never fails since PHP 5.5.0#20
Jan-E wants to merge 1 commit into
php:masterfrom
Jan-E:zend-hash-sort-never-fails

Conversation

@Jan-E

@Jan-E Jan-E commented Jul 12, 2020

Copy link
Copy Markdown
Contributor

@Jan-E

Jan-E commented Jul 12, 2020

Copy link
Copy Markdown
Contributor Author

Compiles fine with PHP 7.4.8 x64 NTS, so should be OK.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 80.252% when pulling cad547a on Jan-E:zend-hash-sort-never-fails into 8422788 on php:master.

1 similar comment
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 80.252% when pulling cad547a on Jan-E:zend-hash-sort-never-fails into 8422788 on php:master.

@Jan-E

Jan-E commented Jul 12, 2020

Copy link
Copy Markdown
Contributor Author

@0mars
The TSRMLS_CC did not do anything mote in PHP7: https://stackoverflow.com/a/49540127/872051
And they are removed in PHP8: https://github.com/php/php-src/blob/master/UPGRADING.INTERNALS#L46
They are partly in the same lines as the zend_has_sort ones, so I cannot yet make a separate PR for those changes. Shall I add them to this PR?

@0mars

0mars commented Jul 12, 2020

Copy link
Copy Markdown
Collaborator

just add them, and we remove them all later. to keep consistency @Jan-E

@Jan-E

Jan-E commented Jul 12, 2020

Copy link
Copy Markdown
Contributor Author

#21
Github says it can merge the PR

@Jan-E

Jan-E commented Jul 12, 2020

Copy link
Copy Markdown
Contributor Author

#21 includes both commits.
#20 only the zend_hash_sort one.


RETURN_FALSE;
}
zend_hash_sort_ex(doc_entry->fields, zend_qsort, comparison_function, renumber TSRMLS_CC);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Simpler to use zend_hash_sort (zend_qsort being dropped in 8)
Other way in pr #22

@Jan-E Jan-E closed this Sep 4, 2020
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.

4 participants