[EMAIL PROTECTED] wrote:
[...]
  -    # test that other length variants - such as constants and
  -    # subroutine returns - don't segfault
  +    # syntax: flatten($p, 0) is equivalent to flatten($p)
       {
  -        my $rc = $bb->flatten(my $data, 300000);
  +        my $data = $bb->flatten($pool, 0);
  +
  +        ok t_cmp(200000,
  +                 length($data),
  +                 'APR::Brigade::flatten() returned all the data');

why? I don't find it intuitive at all. If you say 0 you should get an empty string back (not undef and not the whole brigade).


Not only not intuitive, but wrong. Consider this case: some function returns the wanted amount of chars to slurp, let's say it decides that it doesn't want any more chars and sets $wanted to 0 and you passed it to $bb->flatten($pool, $wanted); now instead of getting an empty string, you get the whole brigade. You could check that $wanted is 0 and not call flatten, but hey why making it more error-prone to users.

I think what you could use undef. These two:

  my $data = $bb->flatten($pool, undef);
  my $data = $bb->flatten($pool);

should probably be the same. But may be we don't need this extra complication at all.

Index: APR__Brigade.h
[...]
  +SV *mpxs_APR__Brigade_flatten(pTHX_ apr_bucket_brigade *bb,
  +                              apr_pool_t *pool, SV *sv_len)
   {
  +
  +    /* XXX we're deviating from the API here to try and make
  +     * the API more Perlish - nobody likes the idea of two
  +     * "in/out" arguments.  and we generally don't ever need
  +     * the length anyway...
  +     */

why the XXX comment? You aren't happy about it?


       apr_status_t status;
  -    apr_size_t len = mp_xs_sv2_apr_size_t(sv_len);
  +    char *buffer;
  +    apr_size_t length;
  +
  +    if (SvTRUE(sv_len)) {

I see, now I understand why you had this 0 case special. You need to manipulate the perl stack here like other '...' functions do. In which case the last argument is optional.


+ return newSVpvn(buffer, length);

this is a bad idea. You should install the buffer into the SV not copy it.


If in server_root_relative it was sort of ok, because usually the string is short there, here you kill the performance and double the memory usage (for a potentially huge string!). I guess to be consistent we need to drop the server_root_relative copying as well and just hope that people will learn to use the right pools if they are used in many APIs.

I'll write on the pool issue in a separate email.

__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]



Reply via email to