# [PATCH] array_fill: Allow an array to specify keys - Thread: https://externals.io/message/24210 - Original archive: https://news-web.php.net/php.internals/24210 - Started by Matt W on 2006-06-25T11:10:54+00:00 - Messages: 14 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #24210 — Matt W — 2006-06-25T11:10:54+00:00 URL: https://externals.io/message/24210#24210 Hi all, I'm on Windows and haven't tried submitting a patch before, so I JUST now installed WinCvs since I don't think you want just a diff of my local file. I used PHP_5_1 since: 5.1's the "regular" download I started working on; if you use it, I assume this can go in 5.1; and I don't know what to do. :-) I've been casually browsing through the PHP source for a while, but never modified it or used C. Just found out about VC++ Express, etc. and set it up to try messing with PHP. Had some missing file errors while building (don't know if that's my problem, or a bug...), but got it done after changing a few files that were causing them. Just some background... ;-) Awhile ago I wished array_fill could also be used like array_fill(array keys, mixed value), which would work like: foreach ($keys as $k) { $array[$k] = $val; } OR $array = array_combine($keys, array_fill(0, count($keys), $val)); So that's what I did for my first, admittedly simple, modification. I couldn't figure out a reason for the *newval variable, since it seemed to be the same as **val (pointer stuff can confuse me, being a C newbie ;-)). Was I wrong to remove it? I also wasn't sure why sizeof(val) was used in one place and sizeof(zval *) in another; but it seems to be random after looking in array.c whether var_name or zval * is used, so I stuck with the latter. I realize now that they're same (right?), but it delayed me, assuming there's a *reason* for everything. Other things I changed while experimenting were initializing the return array AFTER checking the parameters so it doesn't have to be destroyed/freed later; and I made the IS_LONG case first in the switch () for the first parameter's type -- isn't that logical since it's supposed to a PHP int and probably is? :-) I hope it's coded correctly (feedback welcome, as basic as it is, LOL). Everything works like it should as far as I can tell. And it's a lot faster of course than the other 2 methods I mentioned for initializing arbitrary keys. Let me know if I need to diff against a version other than 5.1 or whatever. Thanks, Matt --- ## #24236 — Matt W — 2006-07-01T10:30:48+00:00 URL: https://externals.io/message/24210#24236 · In reply to: #24210 Hi all, I didn't receive any feedback about the first patch I sent (against PHP_5_1), but have since realized that I should just use the MAIN branch, is that correct? Will someone then take care of backporting to older versions if needed...? (Also, from looking at http://cvs.php.net it looks like PHP_5_1 was branched to 5_2 -- sorry about the 5_1 patch. :-/) So here's the patch to extend `array_fill()` for MAIN. Did I get it right this time? :-) Thanks, Matt ----- Original Message ----- > Hi all, > > I'm on Windows and haven't tried submitting a patch before, so I JUST now > installed WinCvs since I don't think you want just a diff of my local file. > I used PHP_5_1 since: 5.1's the "regular" download I started working on; if > you use it, I assume this can go in 5.1; and I don't know what to do. :-) > > I've been casually browsing through the PHP source for a while, but never > modified it or used C. Just found out about VC++ Express, etc. and set it > up to try messing with PHP. Had some missing file errors while building > (don't know if that's my problem, or a bug...), but got it done after > changing a few files that were causing them. Just some background... ;-) > > Awhile ago I wished array_fill could also be used like array_fill(array > keys, mixed value), which would work like: > foreach ($keys as $k) { $array[$k] = $val; } > OR > $array = array_combine($keys, array_fill(0, count($keys), $val)); > > So that's what I did for my first, admittedly simple, modification. > > I couldn't figure out a reason for the *newval variable, since it seemed to > be the same as **val (pointer stuff can confuse me, being a C newbie ;-)). > Was I wrong to remove it? I also wasn't sure why sizeof(val) was used in > one place and sizeof(zval *) in another; but it seems to be random after > looking in array.c whether var_name or zval * is used, so I stuck with the > latter. I realize now that they're same (right?), but it delayed me, > assuming there's a *reason* for everything. > > Other things I changed while experimenting were initializing the return > array AFTER checking the parameters so it doesn't have to be destroyed/freed > later; and I made the IS_LONG case first in the switch () for the first > parameter's type -- isn't that logical since it's supposed to a PHP int and > probably is? :-) > > I hope it's coded correctly (feedback welcome, as basic as it is, LOL). > Everything works like it should as far as I can tell. And it's a lot faster > of course than the other 2 methods I mentioned for initializing arbitrary > keys. Let me know if I need to diff against a version other than 5.1 or > whatever. > > > Thanks, > Matt --- ## #24237 — Marcus Boerger — 2006-07-01T10:53:46+00:00 URL: https://externals.io/message/24210#24237 · In reply to: #24236 Hello Matt, patch looks fine now, once we agree to this set you'd have to provide a patch for 5.2 as well. Another thing we need is tests to ensure all is working as expected. best regards marcus Saturday, July 1, 2006, 12:30:48 PM, you wrote: > Index: ext/standard/array.c > =================================================================== > RCS file: /repository/php-src/ext/standard/array.c,v > retrieving revision 1.350 > diff -u -r1.350 array.c > --- ext/standard/array.c 25 Jun 2006 19:19:31 -0000 1.350 > +++ ext/standard/array.c 1 Jul 2006 09:58:26 -0000 > @@ -1625,46 +1625,83 @@ > Create an array containing num elements starting with index start_key each initialized to val */ > PHP_FUNCTION(array_fill) > { > - zval **start_key, **num, **val, *newval; > + zval **key_data, **num, **val, **entry; > long i; > + HashPosition pos; > > - if (ZEND_NUM_ARGS() != 3 || zend_get_parameters_ex(3, &start_key, &num, &val) == FAILURE) { > + if ((ZEND_NUM_ARGS() != 3 || zend_get_parameters_ex(3, &key_data, &num, &val) == FAILURE) && > + (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &key_data, &val) == FAILURE)) { > WRONG_PARAM_COUNT; > } > > - switch (Z_TYPE_PP(start_key)) { > - case IS_STRING: > - case IS_UNICODE: > - case IS_LONG: > - case IS_DOUBLE: > - /* allocate an array for return */ > - array_init(return_value); > - > - if (PZVAL_IS_REF(*val)) { > - SEPARATE_ZVAL(val); > - } > - convert_to_long_ex(start_key); > - zval_add_ref(val); > - zend_hash_index_update(Z_ARRVAL_P(return_value), > Z_LVAL_PP(start_key), val, sizeof(val), NULL); > - break; > - default: > - php_error_docref(NULL TSRMLS_CC, E_WARNING, "Wrong data type for start key"); > + if (ZEND_NUM_ARGS() == 2) { > + if (Z_TYPE_PP(key_data) != IS_ARRAY) { > + php_error_docref(NULL TSRMLS_CC, E_WARNING, > "First parameter must be an array when passing 2 parameters"); > RETURN_FALSE; > - break; > - } > + } > + } else { > + switch (Z_TYPE_PP(key_data)) { > + case IS_LONG: > + case IS_STRING: > + case IS_UNICODE: > + case IS_DOUBLE: > + convert_to_long_ex(num); > + > + i = Z_LVAL_PP(num) - 1; > + if (i < 0) { > + php_error_docref(NULL TSRMLS_CC, > E_WARNING, "Number of elements must be positive"); > + RETURN_FALSE; > + } > > - convert_to_long_ex(num); > - i = Z_LVAL_PP(num) - 1; > - if (i < 0) { > - zend_hash_destroy(Z_ARRVAL_P(return_value)); > - efree(Z_ARRVAL_P(return_value)); > - php_error_docref(NULL TSRMLS_CC, E_WARNING, "Number of elements must be positive"); > - RETURN_FALSE; > + convert_to_long_ex(key_data); > + break; > + default: > + php_error_docref(NULL TSRMLS_CC, > E_WARNING, "Wrong data type for start key"); > + RETURN_FALSE; > + break; > + } > + } > + > + /* Initialize return array */ > + array_init(return_value); > + > + if (PZVAL_IS_REF(*val)) { > + SEPARATE_ZVAL(val); > } > - newval = *val; > - while (i--) { > - zval_add_ref(&newval); > - zend_hash_next_index_insert(Z_ARRVAL_P(return_value), &newval, sizeof(zval *), NULL); > + > + > + if (Z_TYPE_PP(key_data) == IS_LONG) { > + zval_add_ref(val); > + zend_hash_index_update(Z_ARRVAL_P(return_value), > Z_LVAL_PP(key_data), val, sizeof(zval *), NULL); > + > + while (i--) { > + zval_add_ref(val); > + > zend_hash_next_index_insert(Z_ARRVAL_P(return_value), val, sizeof(zval *), NULL); > + } > + } else { > + > zend_hash_internal_pointer_reset_ex(Z_ARRVAL_PP(key_data), &pos); > + > + while > (zend_hash_get_current_data_ex(Z_ARRVAL_PP(key_data), (void **)&entry, &pos) == SUCCESS) { > + zval_add_ref(val); > + > + if (Z_TYPE_PP(entry) == IS_STRING || > + Z_TYPE_PP(entry) == IS_UNICODE) { > + > zend_u_symtable_update(Z_ARRVAL_P(return_value), Z_TYPE_PP(entry), > Z_UNIVAL_PP(entry), Z_UNILEN_PP(entry) + 1, val, sizeof(zval *), NULL); > + } else if (Z_TYPE_PP(entry) == IS_LONG) { > + > zend_hash_index_update(Z_ARRVAL_P(return_value), Z_LVAL_PP(entry), val, sizeof(zval *), NULL); > + } else { > + zval tmpkey; > + > + tmpkey = **entry; > + zval_copy_ctor(&tmpkey); > + convert_to_string(&tmpkey); > + > + > zend_symtable_update(Z_ARRVAL_P(return_value), Z_STRVAL(tmpkey), > Z_STRLEN(tmpkey) + 1, val, sizeof(zval *), NULL); > + > + zval_dtor(&tmpkey); > + } > + zend_hash_move_forward_ex(Z_ARRVAL_PP(key_data), &pos); > + } > } > } > /* }}} */ Best regards, Marcus --- ## #24272 — Matt W — 2006-07-05T09:25:14+00:00 URL: https://externals.io/message/24210#24272 · In reply to: #24237 Hi Marcus, Thanks for the info. I'm sending along the patch for 5.2 now, since I didn't know whether to wait until the MAIN patch was agreed to (that's what I got from your message), or if you wanted both first. :-) I didn't realize tests were needed for every function, change, etc. (being new at this, sorry). And I don't see any tests for the current `array_fill()`... Anyway, if I need to make a test, how thorough should it be? Should all error messages be checked also, or not, since the message text may change in the future? Are tests needed before the patches will get committed? (Assuming they're agreed to, of course.) ;-) Again, don't know how long that takes either... Thanks, Matt ----- Original Message ----- From: "Marcus Boerger" > Hello Matt, > > patch looks fine now, once we agree to this set you'd have to provide > a patch for 5.2 as well. Another thing we need is tests to ensure all > is working as expected. > > best regards > marcus --- ## #24282 — Marcus Boerger — 2006-07-06T08:17:53+00:00 URL: https://externals.io/message/24210#24282 · In reply to: #24272 Hello Matt, in general we only require head first and once that is in we discuss merging the stuff to older versons (in this case 5.2). Tests have not always been required but it has shown that we introduce to many changes this way. So we more and more develop with tests. Especially when there is a change in an untested area it is very good to test against old documented behavior first and then see what changes with the patch. For the tests themselves we not test for every error situation but we test for a lot. best regards marcus Wednesday, July 5, 2006, 11:25:14 AM, you wrote: > Hi Marcus, > Thanks for the info. I'm sending along the patch for 5.2 now, since I > didn't know whether to wait until the MAIN patch was agreed to (that's what > I got from your message), or if you wanted both first. :-) > I didn't realize tests were needed for every function, change, etc. (being > new at this, sorry). And I don't see any tests for the current > `array_fill()`... Anyway, if I need to make a test, how thorough should it > be? Should all error messages be checked also, or not, since the message > text may change in the future? > Are tests needed before the patches will get committed? (Assuming they're > agreed to, of course.) ;-) Again, don't know how long that takes either... > Thanks, > Matt > ----- Original Message ----- > From: "Marcus Boerger" > > […] Best regards, Marcus --- ## #24326 — Matt W — 2006-07-11T08:54:05+00:00 URL: https://externals.io/message/24210#24326 · In reply to: #24282 Hi Marcus, I'm replying again for clarification about the patch. When you first replied and said it looked OK, you mentioned "once we agree to this," which I assumed meant it could be used/committed. I saw array.c hasn't been changed in CVS (though I don't know when that would happen anyway), so I was kinda wondering, that's all. :-) Then about the tests, I still didn't know whether you'd tell me to make a tests file *after* committing the patch or if you need it *first*. Sorry. :-/ If you (meaning anyone who'd apply the patch) are just waiting for tests, please let me know, and I'll create tests for all `array_fill()` functionality (old and new) right away! BTW, since sending the patches, I realized that my code will allow an empty "keys" array. I'm thinking there should be a warning for that to make it consistent with the old behavior of not returning an empty array. Should I change that and RE-send patches...? Thank you, Matt ----- Original Message ----- From: "Marcus Boerger" > Hello Matt, > > in general we only require head first and once that is in we discuss > merging the stuff to older versons (in this case 5.2). Tests have not > always been required but it has shown that we introduce to many changes > this way. So we more and more develop with tests. Especially when there > is a change in an untested area it is very good to test against old > documented behavior first and then see what changes with the patch. > For the tests themselves we not test for every error situation but we > test for a lot. > > best regards > marcus --- ## #24343 — Marcus Boerger — 2006-07-11T18:37:19+00:00 URL: https://externals.io/message/24210#24343 · In reply to: #24326 Hello Matt, Tuesday, July 11, 2006, 10:54:05 AM, you wrote: > Hi Marcus, > I'm replying again for clarification about the patch. When you first > replied and said it looked OK, you mentioned "once we agree to this," which > I assumed meant it could be used/committed. I saw array.c hasn't been > changed in CVS (though I don't know when that would happen anyway), so I was > kinda wondering, that's all. :-) Well a common problem, you would probably get feedback if you would commit it. Since you don't have CVS rights somebody else needs to do that job but risks to get blamed...so you need to find more people that have interest in your patch. > Then about the tests, I still didn't know whether you'd tell me to make a > tests file *after* committing the patch or if you need it *first*. Sorry. > :-/ First of course. You can do "cvs add ext/standard/tests/array_xyz.phpt" to add your new test file "array_xyz.phpt" virtually to cvs. After that you can regenerate the patch with "cvs di -updN" where -N ensures that those tests go into the patch. > If you (meaning anyone who'd apply the patch) are just waiting for tests, > please let me know, and I'll create tests for all `array_fill()` functionality > (old and new) right away! > BTW, since sending the patches, I realized that my code will allow an empty > "keys" array. I'm thinking there should be a warning for that to make it > consistent with the old behavior of not returning an empty array. Should I > change that and RE-send patches...? Yep, do so please. Best regards, Marcus --- ## #24346 — Andi Gutmans — 2006-07-11T21:42:27+00:00 URL: https://externals.io/message/24210#24346 · In reply to: #24210 I don't have a problem with the functionality and the patch doesn't seem problematic. I do question though whether we want to add this to `array_fill()`? Not sure if that parameter overloading is very intuitive as it doesn't just add one parameter. What do others think? Would you prefer an `array_fill_keys()`? Andi > -----Original Message----- > From: Matt W [mailto:php_lists@realplain.com] > Sent: Sunday, June 25, 2006 4:11 AM > To: internals@lists.php.net > Subject: [PHP-DEV] [PATCH] array_fill: Allow an array to specify keys > > Hi all, > > I'm on Windows and haven't tried submitting a patch before, > so I JUST now installed WinCvs since I don't think you want > just a diff of my local file. > I used PHP_5_1 since: 5.1's the "regular" download I started > working on; if you use it, I assume this can go in 5.1; and I > don't know what to do. :-) > > I've been casually browsing through the PHP source for a > while, but never modified it or used C. Just found out about > VC++ Express, etc. and set it up to try messing with PHP. > Had some missing file errors while building (don't know if > that's my problem, or a bug...), but got it done after > changing a few files that were causing them. Just some > background... ;-) > > Awhile ago I wished array_fill could also be used like > array_fill(array keys, mixed value), which would work like: > foreach ($keys as $k) { $array[$k] = $val; } OR $array = > array_combine($keys, array_fill(0, count($keys), $val)); > > So that's what I did for my first, admittedly simple, modification. > > I couldn't figure out a reason for the *newval variable, > since it seemed to be the same as **val (pointer stuff can > confuse me, being a C newbie ;-)). > Was I wrong to remove it? I also wasn't sure why sizeof(val) > was used in one place and sizeof(zval *) in another; but it > seems to be random after looking in array.c whether var_name > or zval * is used, so I stuck with the latter. I realize now > that they're same (right?), but it delayed me, assuming > there's a *reason* for everything. > > Other things I changed while experimenting were initializing > the return array AFTER checking the parameters so it doesn't > have to be destroyed/freed later; and I made the IS_LONG case > first in the switch () for the first parameter's type -- > isn't that logical since it's supposed to a PHP int and > probably is? :-) > > I hope it's coded correctly (feedback welcome, as basic as it > is, LOL). > Everything works like it should as far as I can tell. And > it's a lot faster of course than the other 2 methods I > mentioned for initializing arbitrary keys. Let me know if I > need to diff against a version other than 5.1 or whatever. > > > Thanks, > Matt --- ## #24366 — Matt W — 2006-07-13T10:28:27+00:00 URL: https://externals.io/message/24210#24366 · In reply to: #24343 Hi Marcus, ----- Original Message ----- From: "Marcus Boerger" > Hello Matt, > > Tuesday, July 11, 2006, 10:54:05 AM, you wrote: > > > […] > > > […] which > > […] was > > […] > > Well a common problem, you would probably get feedback if you would commit > it. Since you don't have CVS rights somebody else needs to do that job but > risks to get blamed...so you need to find more people that have interest > in your patch. Heh, understandable. Can anyone like me get a CVS account if we request it? (I see those request messages often...) Or do you have to be more "known?" :-) Well, even with rights, I'd want to verify a change was OK beforehand -- it sounds like you're saying stuff should be commited *first* and then feedback comes... > > […] a > > […] Sorry. > > […] > > First of course. You can do "cvs add ext/standard/tests/array_xyz.phpt" to > add your new test file "array_xyz.phpt" virtually to cvs. After that you > can regenerate the patch with "cvs di -updN" where -N ensures that those > tests go into the patch. Well, I just tried to use (with WinCvs) "cvs add ..." and it said: "add" requires write access to the repository :-( > > […] tests, > > […] functionality > > […] > > > […] empty > > […] it > > […] Should I > > […] > > Yep, do so please. I created `array_fill_keys()` instead after seeing Andi's e-mail questioning the `array_fill()` modification, and made a few simple tests for it (I see array_fill tests were recently added). So, it's all ready to go, but I can't add the test file... What should I do, e-mail it as a plain file? > Best regards, > Marcus Thanks for your help, Matt --- ## #24386 — Marcus Boerger — 2006-07-15T10:24:37+00:00 URL: https://externals.io/message/24210#24386 · In reply to: #24366 Hello Matt, Thursday, July 13, 2006, 12:28:27 PM, you wrote: > Hi Marcus, > ----- Original Message ----- > From: "Marcus Boerger" > > […] > which > > […] > was > > […] > Heh, understandable. Can anyone like me get a CVS account if we request it? > (I see those request messages often...) Or do you have to be more "known?" > :-) Well, even with rights, I'd want to verify a change was OK > beforehand -- it sounds like you're saying stuff should be commited *first* > and then feedback comes... > > […] > a > > […] > Sorry. > > […] > Well, I just tried to use (with WinCvs) "cvs add ..." and it said: > "add" requires write access to the repository > :-( > > […] > tests, > > […] > functionality > > […] > empty > > […] > it > > […] > Should I > > […] > I created `array_fill_keys()` instead after seeing Andi's e-mail questioning > the `array_fill()` modification, and made a few simple tests for it (I see > array_fill tests were recently added). So, it's all ready to go, but I > can't add the test file... What should I do, e-mail it as a plain file? There's two ways. First as you mentioned sending them as plain text files. And second editing the CVS/Entries file in that directory and adding this line: "/array_fill_keys.phpt/0/dummy timestamp//". However that's for your next patches, i added the function to HEAD and 5.2 after experimenting with it and mofifying it a tiny bit. I allow empty arrays as that is easier for user code. Also this is inline with the ability of `array_fill()` to specify parameter num as zero. Best regards, Marcus --- ## #24389 — Matt W — 2006-07-15T11:25:25+00:00 URL: https://externals.io/message/24210#24389 · In reply to: #24386 Hi Marcus, ----- Original Message ----- From: "Marcus Boerger" > Hello Matt, > > Thursday, July 13, 2006, 12:28:27 PM, you wrote: > > There's two ways. First as you mentioned sending them as plain text files. > And second editing the CVS/Entries file in that directory and adding this > line: "/array_fill_keys.phpt/0/dummy timestamp//". > > However that's for your next patches, i added the function to HEAD and 5.2 > after experimenting with it and mofifying it a tiny bit. I allow empty > arrays as that is easier for user code. Also this is inline with the > ability of `array_fill()` to specify parameter num as zero. > > Best regards, > Marcus Thanks for the CVS info and adding the function! I was just updating the function (and tests) for HEAD when you added it, to make it consistent with the changes Andrei has been making (inc. to array_fill yesterday): http://realplain.com/php/array_fill_keys-1.1.diff http://realplain.com/php/array_fill_keys-1.1.phpt Besides using parse_parameters instead of get_parameters_ex, array_fill also changed the handling of "val" passed by reference -- notice SEPARATE_ZVAL(val) is now gone compared to 5.2. I also marked it "U"nicode compatible, and simplified the code for filling the array, following what Andrei did with array_combine yesterday. I agree about allowing empty arrays, but I was just making it consistent with `array_fill()`, which actually *doesn't* allow parameter "num" as 0: "Number of elements must be positive" Just to clarify what I mean, the code I updated only applies to HEAD (to save Andrei from fixing it ;-)). The 5.2 version is OK I guess (unless you *want* to "simplify" the array filling loop), other than it being inconsistent with array_fill as far as returning an empty array. Thanks again, Matt P.S. Oh, no big deal, but in the NEWS file, you put "Mathew W" instead of "Matt W" or "Matthew W" (2 t's ;-)). And the W is for Wilmas, but the initial is fine; it doesn't matter to me what's there. --- ## #24391 — Marcus Boerger — 2006-07-15T12:14:40+00:00 URL: https://externals.io/message/24210#24391 · In reply to: #24389 Hello Matt, thanks for the update best regards marcus Saturday, July 15, 2006, 1:25:25 PM, you wrote: > Hi Marcus, > ----- Original Message ----- > From: "Marcus Boerger" > > […] > Thanks for the CVS info and adding the function! > I was just updating the function (and tests) for HEAD when you added it, to > make it consistent with the changes Andrei has been making (inc. to > array_fill yesterday): > http://realplain.com/php/array_fill_keys-1.1.diff > http://realplain.com/php/array_fill_keys-1.1.phpt > Besides using parse_parameters instead of get_parameters_ex, array_fill also > changed the handling of "val" passed by reference -- notice > SEPARATE_ZVAL(val) is now gone compared to 5.2. I also marked it "U"nicode > compatible, and simplified the code for filling the array, following what > Andrei did with array_combine yesterday. > I agree about allowing empty arrays, but I was just making it consistent > with `array_fill()`, which actually *doesn't* allow parameter "num" as 0: > "Number of elements must be positive" > Just to clarify what I mean, the code I updated only applies to HEAD (to > save Andrei from fixing it ;-)). The 5.2 version is OK I guess (unless you > *want* to "simplify" the array filling loop), other than it being > inconsistent with array_fill as far as returning an empty array. > Thanks again, > Matt > P.S. Oh, no big deal, but in the NEWS file, you put "Mathew W" instead of > "Matt W" or "Matthew W" (2 t's ;-)). And the W is for Wilmas, but the > initial is fine; it doesn't matter to me what's there. Best regards, Marcus --- ## #24394 — Matt W — 2006-07-15T12:45:16+00:00 URL: https://externals.io/message/24210#24394 · In reply to: #24391 Hi Marcus, ----- Original Message ----- From: "Marcus Boerger" > Hello Matt, > > thanks for the update > > best regards > marcus No problem, but... HEAD looks good now (meaning how you guys want it), but there's an issue with 5.2 -- fill and fill_keys will handle "val" passed by reference differently. Either fill_keys needs if (PZVAL_IS_REF(...)) { SEPARATE_ZVAL(...); } added or that part removed from array_fill to make them behave the same, right? (This is just 5.2, HEAD's fine.) Also, again, with both branches, if you're allowing array_fill_keys to return an empty array (which I support), shouldn't array_fill be changed to do the same? You want consistency where it's expected, right? :-) Thanks, Matt --- ## #24395 — Marcus Boerger — 2006-07-15T13:19:10+00:00 URL: https://externals.io/message/24210#24395 · In reply to: #24394 Hello Matt, Saturday, July 15, 2006, 2:45:16 PM, you wrote: > Hi Marcus, > ----- Original Message ----- > From: "Marcus Boerger" > > […] > No problem, but... > HEAD looks good now (meaning how you guys want it), but there's an issue > with 5.2 -- fill and fill_keys will handle "val" passed by reference > differently. Either fill_keys needs > if (PZVAL_IS_REF(...)) { > SEPARATE_ZVAL(...); > } > added or that part removed from array_fill to make them behave the same, > right? (This is just 5.2, HEAD's fine.) Well the code is equal now. > Also, again, with both branches, if you're allowing array_fill_keys to > return an empty array (which I support), shouldn't array_fill be changed to > do the same? > You want consistency where it's expected, right? :-) Hey, well yeah. I had to read the code more than once to spot how it works in detail and do not particular like the limitation in `array_fill()`. I rather prefer to change that or live with the inconstistency here as the functions more or less do something related but work completley different and have a very different signature, don't they? Best regards, Marcus