Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Add is_countable function #3026

Closed
wants to merge 1 commit into from
Closed

[RFC] Add is_countable function #3026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Zend/zend_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4379,6 +4379,23 @@ ZEND_API zend_bool zend_is_iterable(zval *iterable) /* {{{ */
}
/* }}} */

ZEND_API zend_bool zend_is_countable(zval *countable) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible we should make this zval const *. At a glance it depends if instanceof_function and related calls are const or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, zvals are never passed by const pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it should stay as it is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention likely stems from the days where our zvals directly contained reference counting information. Passing such values by const pointer would prevent making copies on reference counted types, so it wasn't done.

I would prefer it to be const - maybe we should wait for someone else (maybe @dstogov?) to chime in.

Copy link
Contributor

@Majkl578 Majkl578 Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please move this discussion elsewhere (i.e. php.internals) and focus on this patch? We currently have two discussions here both unrelated to the patch itself. Thanks. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Majkl578 You misunderstand. The const discussion is about the patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, zvals are never passed by const pointer.

This really sounds like a global issue for all zvals, not just this patch. Trying to mitigate this convention is out of scope here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my opinion.: it appears instanceof_function takes its parameters by pointer to const and Z_TYPE_P should be const correct as well. This function should take its parameter by pointer to const because we have verified here, today, that it is const correct. It is easier to do this now than later. No other functions need to be adjusted for this function to work with const parameters. As such I believe this is in scope for this PR. If other functions would have to be adjust for it to work then it would be out of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morrisonlevi To avoid this discution on to be const or not to be, should I move this switch to the function declaration, in type.c like #2206?

Also, may I ask you something? What are the difference between have the function here in Zend_api.c or there in type.c?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having it in the API means it can be called from other places in the C code. I think it belongs there - I would not move it inline like the other PR.

{
switch (Z_TYPE_P(countable)) {
case IS_ARRAY:
return 1;
case IS_OBJECT:
if (Z_OBJ_HT_P(countable)->count_elements) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case it will be true?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rentalhost it will be true for extension defined objects that implements their own countable technique

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rentalhost Like @KalleZ said. Also, see @duncan3dc's comment and #2206.

Ref: http://www.phpinternalsbook.com/classes_objects/object_handlers.html#c.count_elements

return 1;
}

return instanceof_function(Z_OBJCE_P(countable), zend_ce_countable);
default:
return 0;
}
}
/* }}} */

/*
* Local variables:
* tab-width: 4
Expand Down
2 changes: 2 additions & 0 deletions Zend/zend_API.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,8 @@ ZEND_API const char *zend_get_object_type(const zend_class_entry *ce);

ZEND_API zend_bool zend_is_iterable(zval *iterable);

ZEND_API zend_bool zend_is_countable(zval *countable);

#define add_method(arg, key, method) add_assoc_function((arg), (key), (method))

ZEND_API ZEND_FUNCTION(display_disabled_function);
Expand Down
1 change: 1 addition & 0 deletions ext/opcache/Optimizer/zend_func_info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ static const func_info_t func_infos[] = {
F0("is_object", MAY_BE_FALSE | MAY_BE_TRUE), // TODO: inline with support for incomplete class
F0("is_scalar", MAY_BE_NULL | MAY_BE_FALSE | MAY_BE_TRUE),
F0("is_callable", MAY_BE_NULL | MAY_BE_FALSE | MAY_BE_TRUE),
F0("is_countable", MAY_BE_NULL | MAY_BE_FALSE | MAY_BE_TRUE),
F0("pclose", MAY_BE_FALSE | MAY_BE_LONG),
F1("popen", MAY_BE_NULL | MAY_BE_FALSE | MAY_BE_RESOURCE),
F0("readfile", MAY_BE_FALSE | MAY_BE_LONG),
Expand Down
5 changes: 5 additions & 0 deletions ext/standard/basic_functions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2586,6 +2586,10 @@ ZEND_END_ARG_INFO()
ZEND_BEGIN_ARG_INFO_EX(arginfo_is_iterable, 0, 0, 1)
ZEND_ARG_INFO(0, var)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO(arginfo_is_countable, 0)
ZEND_ARG_INFO(0, var)
ZEND_END_ARG_INFO()
/* }}} */
/* {{{ uniqid.c */
#ifdef HAVE_GETTIMEOFDAY
Expand Down Expand Up @@ -3111,6 +3115,7 @@ static const zend_function_entry basic_functions[] = { /* {{{ */
PHP_FE(is_scalar, arginfo_is_scalar)
PHP_FE(is_callable, arginfo_is_callable)
PHP_FE(is_iterable, arginfo_is_iterable)
PHP_FE(is_countable, arginfo_is_countable)

/* functions from file.c */
PHP_FE(pclose, arginfo_pclose)
Expand Down
1 change: 1 addition & 0 deletions ext/standard/php_type.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,6 @@ PHP_FUNCTION(is_object);
PHP_FUNCTION(is_scalar);
PHP_FUNCTION(is_callable);
PHP_FUNCTION(is_iterable);
PHP_FUNCTION(is_countable);

#endif
19 changes: 19 additions & 0 deletions ext/standard/tests/general_functions/is_countable_with_classes.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
--TEST--
Test is_countable() function
--CREDITS--
Gabriel Caruso (carusogabriel34@gmail.com)
--FILE--
<?php
var_dump(is_countable(new class extends ArrayIterator {}));
var_dump(is_countable((array) new stdClass()));
var_dump(is_countable(new class implements Countable {
public function count()
{
return count(1, 'foo');
}
}));
?>
--EXPECT--
bool(true)
bool(true)
bool(true)
32 changes: 32 additions & 0 deletions ext/standard/tests/general_functions/is_countable_with_variables.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
--TEST--
Test is_countable() function
--CREDITS--
Gabriel Caruso (carusogabriel34@gmail.com)
--FILE--
<?php
var_dump(is_countable([1, 2, 3]));
var_dump(is_countable((array) 1));
var_dump(is_countable((object) ['foo', 'bar', 'baz']));
var_dump(is_countable());

$foo = ['', []];

if (is_countable($foo)) {
var_dump(count($foo));
}

$bar = null;
if (!is_countable($bar)) {
count($bar);
}
?>
--EXPECTF--
bool(true)
bool(true)
bool(false)

Warning: is_countable() expects exactly 1 parameter, 0 given in %s on line %d
NULL
int(2)

Warning: count(): Parameter must be an array or an object that implements Countable in %s on line %d
14 changes: 14 additions & 0 deletions ext/standard/type.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,20 @@ PHP_FUNCTION(is_iterable)
}
/* }}} */

/* {{{ proto bool is_countable(mixed var)
Returns true if var is countable (array or instance of Countable). */
PHP_FUNCTION(is_countable)
{
zval *var;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_ZVAL(var)
ZEND_PARSE_PARAMETERS_END();

RETURN_BOOL(zend_is_countable(var));
}
/* }}} */

/*
* Local variables:
* tab-width: 4
Expand Down