Re: [PHP-CVS] svn: /php/php-src/ branches/PHP_5_3/NEWS branches/PHP_5_3/ext/spl/spl_heap.c branches/PHP_5_3/ext/spl/tests/bug53588.phpt trunk/ext/spl/spl_heap.c trunk/ext/spl/tests/bug53588.phpt
Hi Gustavo,
Can you review the definition of
http://www.php.net/manual/en/splminheap.compare.php ?
I believe the testcase should have the operands reversed to satisfy this
definition
and so the bug was bogus.
Chris
On 12/21/2010 09:29 AM, Gustavo André dos Santos Lopes wrote:
cataphract Tue, 21 Dec 2010 17:29:14 +
Revision: http://svn.php.net/viewvc?view=revisionrevision=306542
Log:
- Fixed bug #53588 (SplMinHeap bad sorting with custom compare
function).
Bug: http://bugs.php.net/53588 (Assigned) SplMinHeap bad insert sort
Changed paths:
U php/php-src/branches/PHP_5_3/NEWS
U php/php-src/branches/PHP_5_3/ext/spl/spl_heap.c
A php/php-src/branches/PHP_5_3/ext/spl/tests/bug53588.phpt
U php/php-src/trunk/ext/spl/spl_heap.c
A php/php-src/trunk/ext/spl/tests/bug53588.phpt
Modified: php/php-src/branches/PHP_5_3/NEWS
===
--- php/php-src/branches/PHP_5_3/NEWS 2010-12-21 16:05:40 UTC (rev 306541)
+++ php/php-src/branches/PHP_5_3/NEWS 2010-12-21 17:29:14 UTC (rev 306542)
@@ -70,6 +70,8 @@
- SPL extension:
. Fixed bug #53515 (property_exists incorrect on ArrayObject null and 0
values). (Felipe)
+ . Fixed bug #53588 (SplMinHeap bad sorting with custom compare function).
+(Gustavo)
- SQLite extension:
. Fixed memory leaked introduced by the NULL poisoning patch (Mateusz
Kocielski, Pierre)
Modified: php/php-src/branches/PHP_5_3/ext/spl/spl_heap.c
===
--- php/php-src/branches/PHP_5_3/ext/spl/spl_heap.c 2010-12-21 16:05:40 UTC
(rev 306541)
+++ php/php-src/branches/PHP_5_3/ext/spl/spl_heap.c 2010-12-21 17:29:14 UTC
(rev 306542)
@@ -176,7 +176,7 @@
spl_heap_object *heap_object =
(spl_heap_object*)zend_object_store_get_object((zval *)object TSRMLS_CC);
if (heap_object-fptr_cmp) {
long lval = 0;
- if (spl_ptr_heap_cmp_cb_helper((zval *)object, heap_object,
(zval *)a, (zval *)b,lval TSRMLS_CC) == FAILURE) {
+ if (spl_ptr_heap_cmp_cb_helper((zval *)object, heap_object,
(zval *)b, (zval *)a,lval TSRMLS_CC) == FAILURE) {
/* exception or call failure */
return 0;
}
Added: php/php-src/branches/PHP_5_3/ext/spl/tests/bug53588.phpt
===
--- php/php-src/branches/PHP_5_3/ext/spl/tests/bug53588.phpt
(rev 0)
+++ php/php-src/branches/PHP_5_3/ext/spl/tests/bug53588.phpt2010-12-21
17:29:14 UTC (rev 306542)
@@ -0,0 +1,23 @@
+--TEST--
+Bug #53588 (SplMinHeap bad sorting with custom compare function)
+--FILE--
+?php
+class MySimpleHeap extends SplMinHeap{
+public function compare( $value1, $value2 ){
+return ( $value1 - $value2 );
+}
+}
+
+$obj = new MySimpleHeap();
+$obj-insert( 8 );
+$obj-insert( 0 );
+$obj-insert( 4 );
+
+foreach( $obj as $number ) {
+echo $number, \n;
+}
+--EXPECT--
+0
+4
+8
+
Modified: php/php-src/trunk/ext/spl/spl_heap.c
===
--- php/php-src/trunk/ext/spl/spl_heap.c2010-12-21 16:05:40 UTC (rev
306541)
+++ php/php-src/trunk/ext/spl/spl_heap.c2010-12-21 17:29:14 UTC (rev
306542)
@@ -176,7 +176,7 @@
spl_heap_object *heap_object =
(spl_heap_object*)zend_object_store_get_object((zval *)object TSRMLS_CC);
if (heap_object-fptr_cmp) {
long lval = 0;
- if (spl_ptr_heap_cmp_cb_helper((zval *)object, heap_object,
(zval *)a, (zval *)b,lval TSRMLS_CC) == FAILURE) {
+ if (spl_ptr_heap_cmp_cb_helper((zval *)object, heap_object,
(zval *)b, (zval *)a,lval TSRMLS_CC) == FAILURE) {
/* exception or call failure */
return 0;
}
Added: php/php-src/trunk/ext/spl/tests/bug53588.phpt
===
--- php/php-src/trunk/ext/spl/tests/bug53588.phpt
(rev 0)
+++ php/php-src/trunk/ext/spl/tests/bug53588.phpt 2010-12-21 17:29:14 UTC
(rev 306542)
@@ -0,0 +1,23 @@
+--TEST--
+Bug #53588 (SplMinHeap bad sorting with custom compare function)
+--FILE--
+?php
+class MySimpleHeap extends SplMinHeap{
+public function compare( $value1, $value2 ){
+return ( $value1 - $value2 );
+}
+}
+
+$obj = new MySimpleHeap();
+$obj-insert( 8 );
+$obj-insert( 0 );
+$obj-insert( 4 );
+
+foreach( $obj as $number ) {
+echo $number, \n;
+}
+--EXPECT--
+0
+4
+8
+
--
Email: christopher.jo...@oracle.com
Tel: +1 650 506 8630
Blog: http://blogs.oracle.com/opal/
--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit:
Re: [PHP-CVS] svn: /php/php-src/ branches/PHP_5_3/NEWS branches/PHP_5_3/ext/spl/spl_heap.c branches/PHP_5_3/ext/spl/tests/bug53588.phpt trunk/ext/spl/spl_heap.c trunk/ext/spl/tests/bug53588.phpt
On 12/21/2010 10:47 AM, Gustavo Lopes wrote:
On Tue, 21 Dec 2010 18:23:11 -, Christopher Jones
christopher.jo...@oracle.com wrote:
Hi Gustavo,
Can you review the definition of
http://www.php.net/manual/en/splminheap.compare.php ?
I believe the testcase should have the operands reversed to satisfy this
definition and so the bug was bogus.
I did. The definition says compare() should give a positive number if $value1
is larger than $value2, which it does:
That's the definition for SplMaxHeap. The definition for SplMinHeap
is positive integer if value1 is lower than value2. See the link
above.
Chris
+?php
+class MySimpleHeap extends SplMinHeap{
+ public function compare( $value1, $value2 ){
+ return ( $value1 - $value2 );
+ }
+}
+
+$obj = new MySimpleHeap();
+$obj-insert( 8 );
+$obj-insert( 0 );
+$obj-insert( 4 );
+
+foreach( $obj as $number ) {
+ echo $number, \n;
+}
+--EXPECT--
+0
+4
+8
+
So this implementation of compare() gives the usual order and SplMinHeap
should give the elements in the usual increasing order (the top should have the smallest).
Moreover, without the change extending SplMinHeap or SplMaxHeap would give the
same result.
--
Email: christopher.jo...@oracle.com
Tel: +1 650 506 8630
Blog: http://blogs.oracle.com/opal/
--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-CVS] svn: /php/php-src/ branches/PHP_5_3/NEWS branches/PHP_5_3/ext/spl/spl_heap.c branches/PHP_5_3/ext/spl/tests/bug53588.phpt trunk/ext/spl/spl_heap.c trunk/ext/spl/tests/bug53588.phpt
On Tue, 21 Dec 2010 18:23:11 -, Christopher Jones
christopher.jo...@oracle.com wrote:
Hi Gustavo,
Can you review the definition of
http://www.php.net/manual/en/splminheap.compare.php ?
I believe the testcase should have the operands reversed to satisfy this
definition and so the bug was bogus.
I did. The definition says compare() should give a positive number if
$value1 is larger than $value2, which it does:
+?php
+class MySimpleHeap extends SplMinHeap{
+public function compare( $value1, $value2 ){
+return ( $value1 - $value2 );
+}
+}
+
+$obj = new MySimpleHeap();
+$obj-insert( 8 );
+$obj-insert( 0 );
+$obj-insert( 4 );
+
+foreach( $obj as $number ) {
+echo $number, \n;
+}
+--EXPECT--
+0
+4
+8
+
So this implementation of compare() gives the usual order and SplMinHeap
should give the elements in the usual increasing order (the top should
have the smallest).
Moreover, without the change extending SplMinHeap or SplMaxHeap would give
the same result.
--
Gustavo Lopes
--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-CVS] svn: /php/php-src/ branches/PHP_5_3/NEWS branches/PHP_5_3/ext/spl/spl_heap.c branches/PHP_5_3/ext/spl/tests/bug53588.phpt trunk/ext/spl/spl_heap.c trunk/ext/spl/tests/bug53588.phpt
On Tue, 21 Dec 2010 18:52:07 -, Christopher Jones christopher.jo...@oracle.com wrote: On 12/21/2010 10:47 AM, Gustavo Lopes wrote: On Tue, 21 Dec 2010 18:23:11 -, Christopher Jones christopher.jo...@oracle.com wrote: Hi Gustavo, Can you review the definition of http://www.php.net/manual/en/splminheap.compare.php ? I believe the testcase should have the operands reversed to satisfy this definition and so the bug was bogus. I did. The definition says compare() should give a positive number if $value1 is larger than $value2, which it does: That's the definition for SplMaxHeap. The definition for SplMinHeap is positive integer if value1 is lower than value2. See the link above. Oops. You're right. I've reversed the change and closed the bug as bogus. Thanks. -- Gustavo Lopes -- PHP CVS Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
