Skip to content
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.

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

MAINT: Cleanup remaining PyUString_ConcatAndDel use. #17409

Merged
merged 7 commits into from Oct 2, 2020

Conversation

charris
Copy link
Member

@charris charris commented Oct 1, 2020

The build_shape_string function has also been removes as a duplicate of convert_shape_to_string.

This should probably be squash merged.


PyObject *shape2 = convert_shape_to_string(ndim, shape, "");
Copy link
Member

Choose a reason for hiding this comment

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

As a follow-up to this collection of PRs, its very temping to remove the third argument from this function

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. It doesn't get used and is easy enough to add elsewhere if needed.

if (doc == NULL) {
return NULL;
}
// FIXME: check if ufunc docstrings can/should be UTF-8
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the quetion behind this comment - the PR hasn't changed anything, has it?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// FIXME: check if ufunc docstrings can/should be UTF-8

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks good, let's remove that FIXME though

The build_shape_string function is effectively a duplicate of the
convert_shape_to_string function in common.c. All uses have now been
replaced, so remove it.
@charris charris force-pushed the cleanup-remaining-PyUString-use branch from 18bd2da to e6030ff Compare October 1, 2020 13:30
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Well, tried to see if I find anything that might trip PyPy, but didn't manage. On the up-side, the code changes look very nice :).

@charris charris merged commit b93b67c into numpy:master Oct 2, 2020
@charris charris deleted the cleanup-remaining-PyUString-use branch October 2, 2020 15:20
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.

None yet

3 participants