Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [PATCH] array_fill: Allow an array to specify keys »

[PATCH] array_fill: Allow an array to specify keys

php.internals

Matt W

20 years ago
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

Matt W

20 years ago
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

Marcus Börger

20 years ago
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

Matt W

20 years ago
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" <helly@php.net>

Marcus Börger

20 years ago
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" <helly@php.net>
>> 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

Matt W

20 years ago
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"

Marcus Börger

20 years ago
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

Matt W

20 years ago
Hi Marcus, ----- Original Message ----- From: "Marcus Boerger"
> 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.
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...
> > 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.
Well, I just tried to use (with WinCvs) "cvs add ..." and it said: "add" requires write access to the repository :-(
> > 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.
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

Marcus Börger

20 years ago
Hello Matt, Thursday, July 13, 2006, 12:28:27 PM, you wrote:
> Hi Marcus,
> ----- Original Message ----- > From: "Marcus Boerger"
>> 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.
> 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...
>> > 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.
> Well, I just tried to use (with WinCvs) "cvs add ..." and it said:
> "add" requires write access to the repository
> :-(
>> > 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.
> 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

Matt W

20 years ago
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.

Andi Gutmans

20 years ago
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

Matt W

20 years ago
Hi Andi, Thanks for the reply. Yeah, array_fill_keys() may well make more sense. :-) I had thought about that originally, and thought I came up with a reason to just extend array_fill(), but I can't remember the reasoning now... (Maybe I wasn't sure if you'd want another function.) After your message, I think I'll proceed with array_fill_keys unless I hear otherwise. OK? And make tests for it and array_fill (since there are none now) and submit a new patch with everything ASAP. Thanks, Matt ----- Original Message ----- From: "Andi Gutmans" <andi@zend.com> To: "'Matt W'" <php_lists@realplain.com>; <internals@lists.php.net> Sent: Tuesday, July 11, 2006 4:42 PM Subject: RE: [PHP-DEV] [PATCH] array_fill: Allow an array to specify keys