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

Add CPlusPlus support #321

Merged
merged 7 commits into from
May 17, 2022
Merged

Add CPlusPlus support #321

merged 7 commits into from
May 17, 2022

Conversation

mattip
Copy link
Contributor

@mattip mattip commented May 5, 2022

The codebase did not support compiling with a c++ compiler. I also added a CI run for g++

@mattip mattip force-pushed the cpp branch 3 times, most recently from 758a10e to 3ba6e81 Compare May 5, 2022 12:52
@steve-s
Copy link
Contributor

steve-s commented May 5, 2022

If it works I am all for this. Most of the changes look sane even for pure C codebase. But doesn't testing everything with g++ push it farther than necessary? We "only" need the header files C++ compatible (*), i.e., the static inline functions and macros, which get pulled into the C++ code of the C++ extension, but other things that just link with the extension are fine to be C only?

(*) plus the C files that are currently compiled with the extension, but as we discussed in the meeting, we'd like to actually compile them ourselves, so in that case they'd be always compiled by C compiler too).

@mattip
Copy link
Contributor Author

mattip commented May 6, 2022

Closes #318. In order to do this the PR:

  • Adds a CI run using g++ for tests on ubuntu
  • Allows overriding gcc in the test/Makefile
  • Moves HPyTuple_Pack from a macro to an function in inline_helpers.h
  • Adds guards when exporting functions to prevent mangling the names
  • Casts malloc (and friends) results to the proper pointer type
  • Fixes tests to run with C++
    • Casts where needed
    • Move code so variable declarations are not between a goto xxx and the xxx label
    • Reorder HPyType_Spec initializations so that the order matches the struct definition

The new CI run is passing.

@steve-s steve-s self-requested a review May 6, 2022 07:12
@mattip
Copy link
Contributor Author

mattip commented May 7, 2022

Would anyone else like to review or can this be merged?

@qunaibit qunaibit self-requested a review May 7, 2022 19:12
Copy link
Contributor

@qunaibit qunaibit left a comment

Choose a reason for hiding this comment

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

LGTM

@mattip
Copy link
Contributor Author

mattip commented May 11, 2022

ping

@mattip
Copy link
Contributor Author

mattip commented May 15, 2022

This PR has two approvals, can we merge it?

@thedrow thedrow merged commit 02a4f7e into hpyproject:master May 17, 2022
@mattip
Copy link
Contributor Author

mattip commented May 19, 2022

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants