Skip to content

Conversation

@eric-wieser
Copy link
Member

Extracted from #9022

No behavior changes, but makes the __array_wrap__ logic a little clearer.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Modulo the minor comment that is not even strictly part of this PR, this all looks OK. Definitely clearer.

}

/* Call the method with appropriate context */
args_tup = _get_wrap_prepare_args(full_args);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite this PR, but while you're at it: there would seem to be no reason to call this every time in the loop; maybe preset args_tup to NULL on top, and here only set it if NULL? I.e.,

if (args_tup == NULL) {
    args_tup = _get_wrap_prepare_args(full_args);
    if (args_tup == NULL) {
        goto fail;
    }
}

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 also considered just caching this on the full_args object. Something for another PR, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that's fine.

@eric-wieser
Copy link
Member Author

@mhvk, good to go? It would be nice to get this and maybe #9022 in, so that any downstream errors caused by the 1.15 out argument changes to __array_wrap__ are not silenced.

@mhvk mhvk merged commit 671ba6e into numpy:master May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants