-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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: Refactor stringdtype casts.c to use cpp templates #28091
base: main
Are you sure you want to change the base?
MAINT: Refactor stringdtype casts.c to use cpp templates #28091
Conversation
fd3e922
to
3cdcafe
Compare
3cdcafe
to
220615b
Compare
220615b
to
d15e583
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone else wants to look over this carefully, I strongly recommend the side-by-side diff view.
Most of these comments are formatting related. Overall this looks like a very nice improvement.
goto next_step; | ||
in += in_stride; | ||
out += out_stride; | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you're going to change this can you also do the same change in stringdtype_ufuncs.cpp
? Or just leave it, I added the gotos because I was spending far too much time trying to understand the control flow and the repeated code was confusing me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I should have left a note about this in the body of the PR. This change is because the compiler will not allow you to goto
across variable definitions. So with cpp this is actually a compile time error.
I can still make the same changes in stringdtype_ufuncs.cpp
as well so that it matches, if you prefer - but on first look it seems like those jumps don't cross variable definitions.
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Summary
Part of #25693.
This PR replaces several places where macros were being used to generate cast specs for the
vstring
type with c++ templates. No user-facing changes were made.Other changes
but in using
ss#nns
,PyExc_UnicodeEncodeError
expected a string and buffer length for the second and third arguments. I've changed this tosOnns
, to accept abytes
object containing the offending data.Notes
PyArray_DTypeMeta
structs. I'm not sure if there is a better way to do this, but if you have any ideas about this I'd be grateful to learn how this could be done more elegantly (or if these would make sense to have in a different place in the code - I could see them being useful in more places than just this...):typenum_to_shortname_cstr
- I realize not all NPY_TYPES have corresponding shortnames, but these were the ones necessary to finish this efforttypenum_to_cstr
typenum_to_dtypemeta
char
can't be initialized with string literalssize_t
|
, e.g.NPY_METH_NO_FLOATINGPOINT_ERRORS | NPY_METH_REQUIRES_PYAPI
is not of typeNPY_ARRAYMETHOD_FLAGS
, it is of typeint
. Explicit casts were added to ensure the correct typegoto
was jumping over a variable initialization, which isn't allowed by the cpp compiler. The instance was modified to work without the jump.@ngoldbaum I noticed that the string to f128 cast has only the
NPY_METH_NO_FLOATINGPOINT_ERRORS
array method flag. But if you look atstring_to_float
, orstring_to_float_resolve_descriptors
, it looks like there are lines that are accessing python objects, callingPy_INCREF
, etc. I've kept the array flags identical to how they were before, but should this also have aNPY_METH_REQUIRES_PYAPI
flag as well?