19 years ago by Matt W — view source

unread

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

19 years ago by Matt W — view source

unread

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

19 years ago by Marcus Boerger — view source

unread

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

19 years ago by Matt W — view source

unread

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

19 years ago by Marcus Boerger — view source

unread

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

19 years ago by Matt W — view source

unread

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

19 years ago by Marcus Boerger — view source

unread

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

19 years ago by Matt W — view source

unread

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

19 years ago by Marcus Boerger — view source

unread

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

19 years ago by Matt W — view source

unread

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.

19 years ago by Marcus Boerger — view source

unread

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"

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.

Best regards,
Marcus

19 years ago by Matt W — view source

unread

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

19 years ago by Marcus Boerger — view source

unread

Hello Matt,

Saturday, July 15, 2006, 2:45:16 PM, you wrote:

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.)

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

19 years ago by Andi Gutmans — view source

unread

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