Skip to content

PHPC-1073: Remove superfluous calls to php_array_exists #1029

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

Merged
merged 2 commits into from
Oct 18, 2019
Merged

PHPC-1073: Remove superfluous calls to php_array_exists #1029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
910b5e7
PHPC-1073: Remove superfluous calls to php_array_exists
alcaeus Oct 2, 2019
0d767a7
PHPC-1073: Refactor functions to early exit on missing elements
alcaeus Oct 2, 2019
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
18 changes: 6 additions & 12 deletions src/MongoDB/BulkWrite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,12 +242,8 @@ static bool php_phongo_bulkwrite_update_apply_options(bson_t* boptions, zval* zo
bool multi = false, upsert = false;

if (zoptions) {
if (php_array_existsc(zoptions, "multi")) {
multi = php_array_fetchc_bool(zoptions, "multi");
}
if (php_array_existsc(zoptions, "upsert")) {
upsert = php_array_fetchc_bool(zoptions, "upsert");
}
multi = php_array_fetchc_bool(zoptions, "multi");
upsert = php_array_fetchc_bool(zoptions, "upsert");
Copy link
Member

Choose a reason for hiding this comment

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

PHPC-1073 mentions that bool fetching might still want "exists" checks, but I assume this is fine since both default to false already, correct?

Likewise for any bool fetches later on in this PR where false corresponds to the default case.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is correct. Looking at php_array_fetchc_bool, it returns false if the option isn't given. Since these two are false by default, we don't need to check for the existence of it. When we have a bool that is true by default, we need to check for existence first so we can default to true.

}

PHONGO_BULKWRITE_APPEND_BOOL("multi", multi);
Expand All @@ -264,9 +260,7 @@ static bool php_phongo_bulkwrite_delete_apply_options(bson_t* boptions, zval* zo
int32_t limit = 0;

if (zoptions) {
if (php_array_existsc(zoptions, "limit")) {
limit = php_array_fetchc_bool(zoptions, "limit") ? 1 : 0;
}
limit = php_array_fetchc_bool(zoptions, "limit") ? 1 : 0;
}

PHONGO_BULKWRITE_APPEND_INT32("limit", limit);
Expand Down Expand Up @@ -391,7 +385,7 @@ static PHP_METHOD(BulkWrite, update)
}

if (php_phongo_bulkwrite_update_has_operators(&bupdate) || php_phongo_bulkwrite_update_is_pipeline(&bupdate)) {
if (zoptions && php_array_existsc(zoptions, "multi") && php_array_fetchc_bool(zoptions, "multi")) {
if (zoptions && php_array_fetchc_bool(zoptions, "multi")) {
if (!mongoc_bulk_operation_update_many_with_opts(intern->bulk, &bquery, &bupdate, &boptions, &error)) {
phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC);
goto cleanup;
Expand All @@ -403,7 +397,7 @@ static PHP_METHOD(BulkWrite, update)
}
}
} else {
if (zoptions && php_array_existsc(zoptions, "multi") && php_array_fetchc_bool(zoptions, "multi")) {
if (zoptions && php_array_fetchc_bool(zoptions, "multi")) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Replacement document conflicts with true \"multi\" option");
goto cleanup;
}
Expand Down Expand Up @@ -447,7 +441,7 @@ static PHP_METHOD(BulkWrite, delete)
goto cleanup;
}

if (zoptions && php_array_existsc(zoptions, "limit") && php_array_fetchc_bool(zoptions, "limit")) {
if (zoptions && php_array_fetchc_bool(zoptions, "limit")) {
if (!mongoc_bulk_operation_remove_one_with_opts(intern->bulk, &bquery, &boptions, &error)) {
phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC);
goto cleanup;
Expand Down
26 changes: 15 additions & 11 deletions src/MongoDB/Command.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,26 @@ zend_class_entry* php_phongo_command_ce;
* via mongoc_cursor_set_max_await_time_ms(). */
static bool php_phongo_command_init_max_await_time_ms(php_phongo_command_t* intern, zval* options TSRMLS_DC) /* {{{ */
{
if (php_array_existsc(options, "maxAwaitTimeMS")) {
int64_t max_await_time_ms = php_array_fetchc_long(options, "maxAwaitTimeMS");
int64_t max_await_time_ms;

if (max_await_time_ms < 0) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"maxAwaitTimeMS\" option to be >= 0, %" PRId64 " given", max_await_time_ms);
return false;
}
if (!php_array_existsc(options, "maxAwaitTimeMS")) {
return true;
}

if (max_await_time_ms > UINT32_MAX) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"maxAwaitTimeMS\" option to be <= %" PRIu32 ", %" PRId64 " given", UINT32_MAX, max_await_time_ms);
return false;
}
max_await_time_ms = php_array_fetchc_long(options, "maxAwaitTimeMS");

intern->max_await_time_ms = (uint32_t) max_await_time_ms;
if (max_await_time_ms < 0) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"maxAwaitTimeMS\" option to be >= 0, %" PRId64 " given", max_await_time_ms);
return false;
}

if (max_await_time_ms > UINT32_MAX) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"maxAwaitTimeMS\" option to be <= %" PRIu32 ", %" PRId64 " given", UINT32_MAX, max_await_time_ms);
return false;
}

intern->max_await_time_ms = (uint32_t) max_await_time_ms;

return true;
} /* }}} */

Expand Down
46 changes: 27 additions & 19 deletions src/MongoDB/Query.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ static bool php_phongo_query_init_hint(php_phongo_query_t* intern, zval* options
* and default singleBatch to true. */
static bool php_phongo_query_init_limit_and_singlebatch(php_phongo_query_t* intern, zval* options TSRMLS_DC) /* {{{ */
{
if (php_array_existsc(options, "limit") && php_array_fetchc_long(options, "limit") < 0) {
if (php_array_fetchc_long(options, "limit") < 0) {
phongo_long limit = php_array_fetchc_long(options, "limit");

if (!BSON_APPEND_INT64(intern->opts, "limit", -limit)) {
Expand Down Expand Up @@ -203,17 +203,21 @@ static bool php_phongo_query_init_limit_and_singlebatch(php_phongo_query_t* inte
* which must be converted to a mongoc_read_concern_t. */
static bool php_phongo_query_init_readconcern(php_phongo_query_t* intern, zval* options TSRMLS_DC) /* {{{ */
{
if (php_array_existsc(options, "readConcern")) {
zval* read_concern = php_array_fetchc(options, "readConcern");
zval* read_concern;

if (Z_TYPE_P(read_concern) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(read_concern), php_phongo_readconcern_ce TSRMLS_CC)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"readConcern\" option to be %s, %s given", ZSTR_VAL(php_phongo_readconcern_ce->name), PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(read_concern));
return false;
}
if (!php_array_existsc(options, "readConcern")) {
return true;
}

read_concern = php_array_fetchc(options, "readConcern");

intern->read_concern = mongoc_read_concern_copy(phongo_read_concern_from_zval(read_concern TSRMLS_CC));
if (Z_TYPE_P(read_concern) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(read_concern), php_phongo_readconcern_ce TSRMLS_CC)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"readConcern\" option to be %s, %s given", ZSTR_VAL(php_phongo_readconcern_ce->name), PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(read_concern));
return false;
}

intern->read_concern = mongoc_read_concern_copy(phongo_read_concern_from_zval(read_concern TSRMLS_CC));

return true;
} /* }}} */

Expand All @@ -224,22 +228,26 @@ static bool php_phongo_query_init_readconcern(php_phongo_query_t* intern, zval*
* via mongoc_cursor_set_max_await_time_ms(). */
static bool php_phongo_query_init_max_await_time_ms(php_phongo_query_t* intern, zval* options TSRMLS_DC) /* {{{ */
{
if (php_array_existsc(options, "maxAwaitTimeMS")) {
int64_t max_await_time_ms = php_array_fetchc_long(options, "maxAwaitTimeMS");
int64_t max_await_time_ms;

if (max_await_time_ms < 0) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"maxAwaitTimeMS\" option to be >= 0, %" PRId64 " given", max_await_time_ms);
return false;
}
if (!php_array_existsc(options, "maxAwaitTimeMS")) {
return true;
}

if (max_await_time_ms > UINT32_MAX) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"maxAwaitTimeMS\" option to be <= %" PRIu32 ", %" PRId64 " given", UINT32_MAX, max_await_time_ms);
return false;
}
max_await_time_ms = php_array_fetchc_long(options, "maxAwaitTimeMS");

intern->max_await_time_ms = (uint32_t) max_await_time_ms;
if (max_await_time_ms < 0) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"maxAwaitTimeMS\" option to be >= 0, %" PRId64 " given", max_await_time_ms);
return false;
}

if (max_await_time_ms > UINT32_MAX) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"maxAwaitTimeMS\" option to be <= %" PRIu32 ", %" PRId64 " given", UINT32_MAX, max_await_time_ms);
return false;
}

intern->max_await_time_ms = (uint32_t) max_await_time_ms;

return true;
} /* }}} */

Expand Down