ext/com_dotnet: Use modern HashTable APIs #16208
Conversation
Girgias
force-pushed
the
com-hash-foreach
branch
from
474aee5 to
d90ab53
Compare
|
Thank you for working on this; long overdue! However, this PR might open up a can of worms. :) I need to check that thoroughly. |
|
I'll let you work on the fixes :) Do ping me when you are done so that I can re-request a review after I fixed all the conflicts. |
Girgias
force-pushed
the
com-hash-foreach
branch
2 times, most recently
from
362f64a to
215915e
Compare
|
@cmb69 I rebased on top of master, hopefully I didn't break anything. |
Girgias
force-pushed
the
com-hash-foreach
branch
from
215915e to
86e7e5d
Compare
There was a problem hiding this comment.
I've left comments regarding the changes to com_variant.c below.
I have not found out why two tests are failing due to the changes to com_wrapper.c (generate_dispids()). Can probably have a closer look this evening.
Regarding the changes to php_com_wrapper_export_as_sink(): I still haven't come around to understanding what this is about. Will try to catch up.
ext/com_dotnet/com_variant.c
Outdated
| for (;; zend_hash_move_forward_ex(Z_ARRVAL_P(z), &pos)) { | ||
|
|
||
| keytype = zend_hash_get_current_key_ex(Z_ARRVAL_P(z), &strindex, &intindex, &pos); | ||
| if (!zend_array_is_list(ht)) { |
There was a problem hiding this comment.
This is now stricter as before, where arrays with holes could be passed. Since the old behavior doesn't make much sense (would then skip elements at the end), the change might be okay, but the test case would need to be updated.
There was a problem hiding this comment.
I'll copy the similar function from f90323c
There was a problem hiding this comment.
That would solve the BC problem, but I'm not sure that's the best way forward.
php-src/ext/com_dotnet/tests/variant_variation.phpt
Lines 26 to 28 in 6deaaf6
I mean, that doesn't make sense. Might be better to require users to pass a proper list (can be checked upfront, or just pass array_values($my_arr)). At least for PHP 9, this should be done.
ext/com_dotnet/com_variant.c
Outdated
| break; | ||
| } | ||
| uint32_t nb_values = zend_hash_num_elements(ht); | ||
| if (nb_values > UINT_MAX) { |
There was a problem hiding this comment.
This appears to be superfluous, since UINT_MAX == UINT32_MAX for all supported platforms. (Actually, that should have been ULONG_MAX, but that doesn't make a difference on Windows.)
I cannot test this locally, nor do I know if we have tests. But I have done a couple of these conversions and it looks correct to me.