Re: [RFC] Simplified Array API for extensions

On 04/03/2013 08:43 PM, Joe Watkins wrote:

On 04/03/2013 06:23 PM, Sara Golemon wrote:

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

Hi Sara, A logical extension of this idea would be to drop _array_ and cover objects too, one uniform everything API is very appealing, and way easier to document properly. Something alone the lines of: static inline zend_bool php_exists(zval *pzval, const char *key) {

         switch (Z_TYPE(pzval)) {
             case IS_ARRAY:
                 return zend_symtable_exists(Z_ARRVAL_P(pzval), key,

strlen(key) + 1);

             default: {
                 if (Z_OBJ_HT_P(pzval)->has_property) {
                     return Z_OBJ_HT_P(pzval)->has_property(
                         pzval, key, strlen(key) + 1, 2 NULL TSRMLS_CC
                     );
                 } else {
                     return zend_symtable_exists(
                         Z_OBJPROP_P(pzval), key, strlen(key) + 1);
                 }
             }
         }

} Just a thought .. Joe

https://gist.github.com/krakjoe/5304945 I'd be happy to complete/test it if we think it's a worthy direction to go in ... a unified api does look delicious, it's a bit more complex than arrays but then in extensions you want/need to deal with objects as much as, if note more than, you do arrays ... Joe