Skip to content

MAINT: Improve const-correctness of string arguments#15254

Merged
mattip merged 1 commit intonumpy:masterfrom
eric-wieser:improve-string-const-correctness
Jan 6, 2020
Merged

MAINT: Improve const-correctness of string arguments#15254
mattip merged 1 commit intonumpy:masterfrom
eric-wieser:improve-string-const-correctness

Conversation

@eric-wieser
Copy link
Member

Using CI to test here because windows compilers aren't strict enough when built by runtests.py

@eric-wieser eric-wieser added 03 - Maintenance 63 - C API Changes or additions to the C API. Mailing list should usually be notified. labels Jan 6, 2020
@eric-wieser eric-wieser force-pushed the improve-string-const-correctness branch 3 times, most recently from 6579420 to 7229c46 Compare January 6, 2020 10:57
@eric-wieser eric-wieser marked this pull request as ready for review January 6, 2020 11:26
{
Py_ssize_t ucs4len = size / sizeof(npy_ucs4);
npy_ucs4 *buf = (npy_ucs4 *)src;
/* FIXME: This is safe, but better to rewrite to not cast away const */
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't want to do this rewrite as part of this patch, as it would be hidden amongst the noise

Comment on lines +709 to +712
/* FIXME: Casting away const makes the below easier to write, but should
* still be safe.
*/
npy_ucs4 *s1t = (npy_ucs4 *)s1, *s2t = (npy_ucs4 *)s2;
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above, didn't want to hide a rewrite amongst a patch that introduces const repeatedly and nothing else

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.

LGTM, feel free to merge, I will probably do later. Could only try to nitpick comment styles or so, and I do not think its worth it.
Why did you tag it C-API, there is no change here? And if, its an additional const qualifier which is fully compatible (or at least only a warning downstream can adapt to trivially).

@eric-wieser
Copy link
Member Author

This is an enhancement to a few public parts of the c API, in that it allows const strings to be passed where previously the could not be. Is the label reserved for incompatible changes?

@seberg
Copy link
Member

seberg commented Jan 6, 2020

Dunno, I suppose not. We have not used it much before anyway? I thought to use it mostly for things were people interested in API decisions (but not technical aspects) may want to have a look. And this felt probably unnecessary.

@mattip mattip merged commit b91dec1 into numpy:master Jan 6, 2020
@mattip
Copy link
Member

mattip commented Jan 6, 2020

Thanks @eric-wieser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

03 - Maintenance 63 - C API Changes or additions to the C API. Mailing list should usually be notified. component: numpy._core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants