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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
