Skip to content

[soon-to-be-rfc?] Implement GH-10526: array_unique: add way to compare items with an identity check (===) #10567

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

Closed
wants to merge 1 commit into from
Closed

[soon-to-be-rfc?] Implement GH-10526: array_unique: add way to compare items with an identity check (===) #10567

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
85 changes: 85 additions & 0 deletions ext/standard/array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,25 @@ static int php_array_key_compare_string_natural_general(Bucket *f, Bucket *s, in
}
/* }}} */

static zend_always_inline int php_array_key_compare_strict_unstable_i(Bucket *f, Bucket *s) /* {{{ */
{
if (f->key == NULL) {
if (s->key == NULL) {
return (zend_long) f->h > (zend_long) s->h ? 1 : -1;
} else {
/* f's keytype is int, s's keytype is string, and int < string */
return -1;
}
} else {
if (s->key == NULL) {
/* f's keytype is string, s's keytype is int, and string > int */
return 1;
} else {
return zendi_smart_strcmp(f->key, s->key);
}
}
}

static int php_array_key_compare_string_natural_case(Bucket *a, Bucket *b) /* {{{ */
{
RETURN_STABLE_SORT(a, b, php_array_key_compare_string_natural_general(a, b, 1));
Expand Down Expand Up @@ -312,6 +331,45 @@ static zend_always_inline int php_array_data_compare_string_unstable_i(Bucket *f
}
/* }}} */

static zend_always_inline int php_array_data_compare_strict_unstable_i(Bucket *f, Bucket *s) /* {{{ */
{
/* We must first check for identity before using a type-juggly zend_compare.
* Otherwise there would be cases which are inconsistent; e.g. `null == false`
* but `null !== false` and we want the latter behaviour. */
if (fast_is_identical_function(&f->val, &s->val)) {
return 0;
}

uint32_t type1 = Z_TYPE(f->val);
uint32_t type2 = Z_TYPE(s->val);

if (type1 == type2) {
return zend_compare(&f->val, &s->val);
Copy link

Choose a reason for hiding this comment

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

I think zvals should be dereferenced before comparison, otherwise we will end up with different types being compared against each other - which we wanted to avoid.

php-src/Zend/zend_operators.c

Lines 2244 to 2250 in 4ec3664

if (Z_ISREF_P(op1)) {
op1 = Z_REFVAL_P(op1);
continue;
} else if (Z_ISREF_P(op2)) {
op2 = Z_REFVAL_P(op2);
continue;
}

Copy link
Member Author

@nielsdos nielsdos Feb 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you're right. I forgot the fact that &int(x)===int(x)

} else {
/* If the types are not equal, we will sort in a way that groups the values of the same
* type together, and within those groups the values are sorted.
* We determine the order of the types based on the alphabetical order (as exposed to userland!),
* which is consistent with the inequality comparisons of `zend_compare` (e.g. false < object < true etc.)
* Order: array, false, double (float), long (int), null, object, reference, resource, string, true.
* Note that double comes before long because the name exposed to userland is float which comes alphabetically
* before long. */
static const signed char type_to_order_mapping[] = {
[IS_ARRAY] = 0,
[IS_FALSE] = 1,
[IS_DOUBLE] = 2,
[IS_LONG] = 3,
[IS_NULL] = 4,
[IS_OBJECT] = 5,
[IS_REFERENCE] = 6,
[IS_RESOURCE] = 7,
[IS_STRING] = 8,
[IS_TRUE] = 9,
};

return ZEND_NORMALIZE_BOOL(type_to_order_mapping[type1] - type_to_order_mapping[type2]);
Comment on lines +356 to +369
Copy link

Choose a reason for hiding this comment

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

We determine the order of the types based on the alphabetical order (as exposed to userland!)

Creating order mapping is kind of wasteful, and ordering by type id looks more logical in my opinion (less complex < more complex):

php-src/Zend/zend_types.h

Lines 549 to 558 in 4ec3664

#define IS_UNDEF 0
#define IS_NULL 1
#define IS_FALSE 2
#define IS_TRUE 3
#define IS_LONG 4
#define IS_DOUBLE 5
#define IS_STRING 6
#define IS_ARRAY 7
#define IS_OBJECT 8
#define IS_RESOURCE 9

Final order doesn't matter anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't agree with this one tbh. The order at least has to be consistent with the inequality ordering of zend_compare (e.g. false<object<true)

Copy link

Choose a reason for hiding this comment

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

There is absolutely no reason to be consistent with zend_compare, that function does non-strict comparisons, which is the opposite of what PR is trying to achieve.

Also I just realized that strings are compared using zendi_smart_strcmp which tries to parse numerical strings, so given values "1e2" and "100", the new flag will remove one of them. Another problematic case would be an array: ["1e2"], ["100"]. I don't think zend_compare should be used after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is absolutely no reason to be consistent with zend_compare, that function does non-strict comparisons, which is the opposite of what PR is trying to achieve.

Yes, but that's not a good reason imo to introduce more inconsistency. The order does not matter like you said, so why not be consistent with zend_compare anyway?

Also I just realized that strings are compared using zendi_smart_strcmp which tries to parse numerical strings, so given values "1e2" and "100", the new flag will remove one of them. Another problematic case would be an array: ["1e2"], ["100"]. I don't think zend_compare should be used after all.

Ugh that's annoying. Fortunately we can just special-case strings.

}
}

static int php_array_natural_general_compare(Bucket *f, Bucket *s, int fold_case) /* {{{ */
{
zend_string *tmp_str1, *tmp_str2;
Expand Down Expand Up @@ -349,11 +407,13 @@ DEFINE_SORT_VARIANTS(key_compare_numeric);
DEFINE_SORT_VARIANTS(key_compare_string_case);
DEFINE_SORT_VARIANTS(key_compare_string);
DEFINE_SORT_VARIANTS(key_compare_string_locale);
DEFINE_SORT_VARIANTS(key_compare_strict);
DEFINE_SORT_VARIANTS(data_compare);
DEFINE_SORT_VARIANTS(data_compare_numeric);
DEFINE_SORT_VARIANTS(data_compare_string_case);
DEFINE_SORT_VARIANTS(data_compare_string);
DEFINE_SORT_VARIANTS(data_compare_string_locale);
DEFINE_SORT_VARIANTS(data_compare_strict);
DEFINE_SORT_VARIANTS(natural_compare);
DEFINE_SORT_VARIANTS(natural_case_compare);

Expand Down Expand Up @@ -408,6 +468,14 @@ static bucket_compare_func_t php_get_key_compare_func(zend_long sort_type, int r
}
break;

case PHP_SORT_STRICT:
if (reverse) {
return php_array_reverse_key_compare_strict;
} else {
return php_array_key_compare_strict;
}
break;

case PHP_SORT_REGULAR:
default:
if (reverse) {
Expand Down Expand Up @@ -472,6 +540,14 @@ static bucket_compare_func_t php_get_data_compare_func(zend_long sort_type, int
}
break;

case PHP_SORT_STRICT:
if (reverse) {
return php_array_reverse_data_compare_strict;
} else {
return php_array_data_compare_strict;
}
break;

case PHP_SORT_REGULAR:
default:
if (reverse) {
Expand Down Expand Up @@ -536,6 +612,14 @@ static bucket_compare_func_t php_get_data_compare_func_unstable(zend_long sort_t
}
break;

case PHP_SORT_STRICT:
if (reverse) {
return php_array_reverse_data_compare_strict_unstable;
} else {
return php_array_data_compare_strict_unstable;
}
break;

case PHP_SORT_REGULAR:
default:
if (reverse) {
Expand Down Expand Up @@ -5653,6 +5737,7 @@ PHP_FUNCTION(array_multisort)
case PHP_SORT_STRING:
case PHP_SORT_NATURAL:
case PHP_SORT_LOCALE_STRING:
case PHP_SORT_STRICT:
/* flag allowed here */
if (parse_state[MULTISORT_TYPE] == 1) {
/* Save the flag and make sure then next arg is not the current flag. */
Expand Down
5 changes: 5 additions & 0 deletions ext/standard/basic_functions.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@
* @cvalue PHP_SORT_NATURAL
*/
const SORT_NATURAL = UNKNOWN;
/**
* @var int
* @cvalue PHP_SORT_STRICT
*/
const SORT_STRICT = UNKNOWN;
/**
* @var int
* @cvalue PHP_SORT_FLAG_CASE
Expand Down
3 changes: 2 additions & 1 deletion ext/standard/basic_functions_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions ext/standard/php_array.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ PHPAPI bool php_array_pick_keys(const php_random_algo *algo, php_random_status *
#define PHP_SORT_ASC 4
#define PHP_SORT_LOCALE_STRING 5
#define PHP_SORT_NATURAL 6
#define PHP_SORT_STRICT 7
#define PHP_SORT_FLAG_CASE 8

#define PHP_COUNT_NORMAL 0
Expand Down
Loading