Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:66921
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: softfail (pb1.pair.com: domain golemon.com does not designate 209.85.160.41 as permitted sender)
MIME-Version: 1.0
Sender: php@golemon.com
In-Reply-To: <CAF+90c9N81ZoSpwm2hSh=gCYV2Y3jB1e+N4_N_d_QfHk9JPK5g@mail.gmail.com>
References: <CAESVnVr_jLwRbVsOOx3urPv0Qzc9Rc-7y2CNp1T0mH+8ad3cRg@mail.gmail.com>
	<CAF+90c9N81ZoSpwm2hSh=gCYV2Y3jB1e+N4_N_d_QfHk9JPK5g@mail.gmail.com>
Date: Wed, 3 Apr 2013 10:23:49 -0700
Message-ID: <CAESVnVrAV4GwhbtUwxez4sAwPboZ1dh=vB2WcW18sv3zt3+MTQ@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [PHP-DEV] [RFC] Simplified Array API for extensions
From: pollita@php.net (Sara Golemon)

>  1a) The "c" modifier seems like an unnecessary microoptimization. Compilers
> should be able to optimize strlen() calls on constant strings away and even
> if they didn't, it wouldn't be much of a big deal. Also using the
> "c"-variants on a non-literal would not throw an error and just use an
> invalid length instead.
>
Yeah, killing this for sure.

>  1b) Imho the "l_safe" case does not need its own modifier. Typically
> strings in PHP are always zero-terminated (in debug mode you'll actually get
> warnings if they are not). The cases where they aren't zero-terminated are
> rare and in these cases one can just write those two extra lines of code to
> do the right thing.
>
Yeah, the safe version wasn't meant to capture a common case, but it
is one I've found myself running into more a little commonly.  The
argument that the rest of the runtime doesn't provide this affordance
is a good one, but I'm still on the fence for this one.

>   c) I think it would be a lot more intuitive if we used the terminology of
> the normal array APIs instead of the shorthands:
>
>     php_array_fetch => php_array_fetch_assoc
>     php_array_fetchl => php_array_fetch_assoc_ex
>     php_array_fetchn => php_array_fetch_index
>     php_array_fetchz => php_array_fetch_zval
>
+1 for consistency, combined with separating out the type conversion
bits the verbosity (which I was trying to avoid) is also not so bad.

> 2. The php_array_fetch*_* APIs currently combined two things: a) Fetching
> from the array and b) Casting it to some type. I think both should be
> separate. Not only to avoid the combinatorial explosion of different
> modifier+type combinations, but also because those casting methods are also
> applicable to other contexts, not just arrays casts. I asked some time ago
> to add functions that can directly get a long/double from a zval (though
> didn't pursue this further). Your APIs add something like that, but tightly
> coupled to array fetches. There would be more use for it if it were separate
> :)
>
It's outside the scope of what I was originally trying to accomplish,
but it's got utility in its own right.  So I'll rewrite this entire
proposal as two separate tasks:

#1 - Array access APIs for fetching zval*

  zend_array_(fetch|exists|unset)_(assoc|assocl|index|zval)()

#1a - (Optional) Array add APIs renamed for consistency and namespacing:
  I'm not a huge fan of this as we'd need to keep
add_(assoc|(next_)?index)_{$type}(), but it's the sort of moment to
embrace consistency in the hopes of deprecating the old calls when we
hit 6.0

#1b - (Optional) Some kind of foreach structure which specializes
zend_hash_apply for the array case:
  void callback(zval *key, zval *val, void *optionalArg) {
    /* key as a zval owned by the iterator (is_ref=0,rc=2 - force
callee to copy if they want to keep it)
       zend_hash_key might be more "to the point", but zval is a much
more familiar structure */
    /* user code here, none of that ZEND_HASH_APPLY_KEEP stuff though */
  }

  zend_array_apply(arr, callback, optionalArg);

#2 Scalar zval* accessors (with implicit type conversions):

  zend_(bool|long|double|string)_value()
  zend_is_true() technically covers zend_bool_value() already, but for
consistency...

And yes, I think we should move em down to Zend...

-Sara