Req #52874 [Opn->Csd]: Refactor big if condition w/ collateral effects and nested ternary statements

Edit report at https://bugs.php.net/bug.php?id=52874&edit=1 ID: 52874 Updated by: cmb@php.net Reported by: cataphract@php.net Summary: Refactor big if condition w/ collateral effects and nested ternary statements -Status: Open +Status: Closed Type: Feature/Change Request Package: Scripting Engine problem Operating System: Irrelevant PHP Version: trunk-SVN-2010-09-18 (SVN) -Assigned To: +Assigned To: cmb Block user comment: N Private report: N New Comment: Apparently, the respective code has been already modified, so I'm closing this ticket. Previous Comments: ------------------------------------------------------------------------ [2010-09-18 03:31:53] cataphract@php.net It's not just non-obvious. It also makes debugging harder. If I segfault on the if condition, my debugger will show it as occurring in the last line of the if. ------------------------------------------------------------------------ [2010-09-18 03:27:51] cataphract@php.net The following patch has been added/updated: Patch Name: refact_if Revision: 1284773271 URL: http://bugs.php.net/patch-display.php?bug=52874&patch=refact_if&revision=1284773271 ------------------------------------------------------------------------ [2010-09-18 03:27:28] cataphract@php.net Description: ------------ Two if conditions (one in zend_std_read_property and another in zend_std_write_property) are very confusing and should be refactored. Here's one: 493 if (EXPECTED(property_info != NULL) && 494 ((EXPECTED((property_info->flags & ZEND_ACC_STATIC) == 0) && 495 property_info->offset >= 0) ? 496 (zobj->properties ? 497 ((variable_ptr = (zval**)zobj->properties_table[property_info->offset]) != NULL) : 498 (*(variable_ptr = &zobj->properties_table[property_info->offset]) != NULL)) : 499 (EXPECTED(zobj->properties != NULL) && 500 EXPECTED(zend_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &variable_ptr) == SUCCESS)))) { 501 /* if we already have this value there, we don't actually need to do anything */ This is very non-obvious. I've refactored the conditions, which I tested with no regressions in the tests. ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=52874&edit=1