8000 MAINT: Convert scalar_math.c.src to scalarmath.cpp.src by serge-sans-paille · Pull Request #21486 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Convert scalar_math.c.src to scalarmath.cpp.src #21486

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

MAINT: Convert scalar_math.c.src to scalarmath.cpp.src #21486

wants to merge 2 commits into from

Conversation

serge-sans-paille
Copy link
Contributor

No description provided.

@serge-sans-paille
[road-to-cxx] scalarmath.c.src -> scalarmath.cpp.src
e45443e
@serge-sans-paille
[road-to-cxx] Convert CONVERT_TO_RESULT to npy_static_cast
e33d472 
npy_static_cast<T>() is roughly equivalent to a static_cast<T>() but it's
designed so that it can be non-intrusively extended for numpy types.

In particular as npy_half is just a type alias on uint16_t, we cannot
speciailize behavior on its type without impacting npy_ushort. so use the
tagging system already used for other C++ parts instead. Ultimately, it would be
great to make npy_half a regular struct to avoid this kind of situation.
@serge-sans-paille
Copy link
Contributor Author

npy_static_cast<T>() is roughly equivalent to a static_cast<T>() but it's
designed so that it can be non-intrusively extended for numpy types.

In particular as npy_half is just a type alias on uint16_t, we cannot
speciailize behavior on its type without impacting npy_ushort. so use the
tagging system already used for other C++ parts instead. Ultimately, it would be
great to make npy_half a regular struct to avoid this kind of situation.

@serge-sans-paille serge-sans-paille mentioned this pull request May 10, 2022
Open
@seberg
Copy link
Member
seberg commented May 10, 2022

Would it make sense to create a second name for npy_half? Or just use define it different for an INTERNAL_BUILD, I would expect there are places where a cast to uint16 is necessary, but I would it is few (and frankly, it will make things cleaner probably).

If that is all that is necessary to avoid the tags, that seems worthwhile?

@serge-sans-paille
Copy link
Contributor Author

Would it make sense to create a second name for npy_half? Or just use define it different for an INTERNAL_BUILD

I don't know what's an INTERNAL_BUILD, but using a

struct npy_half {
  uint16_t value;
}

should have no cost on execution time and would make life easier (and code cleaner).

@serge-sans-paille
Copy link
Contributor Author

I can do that in a separate PR if that's fine with you.

@seberg
Copy link
Member
seberg commented May 10, 2022

@serge-sans-paille I was thinking of just abusing these defines:

#define _UMATHMODULE
#define _MULTIARRAYMODULE

which are used in our public headers to change things a bit. We could even just create a new define which we just always set for NumPy itself (i.e. when one of those two is set)?
If we create a new definition, it would be good to add it to the docs here.

I can do that in a separate PR if that's fine with you.

Yeah, doing that in a seperate PR first seems like the right thing to me also.

@serge-sans-paille
Copy link
Contributor Author

Yeah, doing that in a seperate PR first seems like the right thing to me also.

#21487

@charris charris changed the title Feature/road to cxx scalarmath MAINT: Convert scalar_math.c.src to scalarmath.cpp.src May 10, 2022
@charris charris added 03 - Maintenance C++ Related to introduction and use of C++ in the NumPy code base labels May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
03 - Maintenance C++ Related to introduction and use of C++ in the NumPy code base
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@serge-sans-paille @seberg @charris
0