Skip to content

C-API: Add fix for Windows and C++ #14273

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
Merged
merged 15 commits into from
May 9, 2025
Merged

C-API: Add fix for Windows and C++ #14273

merged 15 commits into from
May 9, 2025

Conversation

doichanj
Copy link
Collaborator

@doichanj doichanj commented Apr 30, 2025 •
edited
Loading

Summary

This is fix for C-API cbindgen of qiskit.h

Details and comments

An environment variable OS is defined on Windows, so I added ifdef OS to avoid overwriting with uname
MSVC generates library whose name is not starting with lib, so set C_LIB_CARGO_BASENAME=qiskit_cext for Windows

Replaced complex type definition with std::complex for C++

Add support for C++ (cpp_compat = true) that defines data types to avoid C style typedef redefinition error.

@doichanj doichanj requested a review from a team as a code owner April 30, 2025 09:53
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Apr 30, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 14923581573

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.01%) to 88.723%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.23%
crates/qasm2/src/lex.rs 5 91.98%
crates/qasm2/src/parse.rs 6 97.15%
Totals Coverage Status
Change from base Build 14915004588: -0.01%
Covered Lines: 76040
Relevant Lines: 85705
💛 - Coveralls

@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Apr 30, 2025
@mtreinish mtreinish added this to the 2.0.1 milestone Apr 30, 2025
@mtreinish mtreinish added Changelog: None Do not include in changelog C API Related to the C API labels Apr 30, 2025
Cryoris
Cryoris reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Adding C++ compatibility is a good idea 👍🏻 I was wondering whether we shouldn't use std::complex in C++ though, since it should be supported on all platforms?

@doichanj
Copy link
Collaborator Author

doichanj commented May 1, 2025

Adding C++ compatibility is a good idea 👍🏻 I was wondering whether we shouldn't use std::complex in C++ though, since it should be supported on all platforms?

I think std::complex is portable for C++ and it works correctly

Cryoris
Cryoris reviewed May 7, 2025
View reviewed changes
Cryoris
Cryoris reviewed May 7, 2025
View reviewed changes
Cryoris
Cryoris reviewed May 7, 2025
View reviewed changes
@mtreinish mtreinish removed the stable backport potential The bug might be minimal and/or import enough to be port to stable label May 8, 2025
@mtreinish mtreinish modified the milestones: 2.0.1, 2.1.0 May 8, 2025
Cryoris
Cryoris reviewed May 8, 2025
View reviewed changes
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM, I verified this works with C++ out of the box. I only have one comment regarding the reno above, but then we can merge from my side 👍🏻

@doichanj @Cryoris
Update releasenotes/notes/fix-c-api-for-windows-and-cpp-0b8d79cd3fcf9…
9c715d9 
…75a.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Cryoris
Cryoris previously approved these changes May 9, 2025
View reviewed changes
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Thanks for the addition!

@Cryoris Cryoris added Changelog: New Feature Include in the "Added" section of the changelog Changelog: Bugfix Include in the "Fixed" section of the changelog and removed Changelog: None Do not include in changelog labels May 9, 2025
@Cryoris Cryoris added this pull request to the merge queue May 9, 2025
Merged via the queue into Qiskit:main with commit 7c04a47 May 9, 2025
25 of 26 checks passed
rahaman-quantum pushed a commit to rahaman-quantum/qiskit that referenced this pull request Jun 20, 2025
@doichanj @Cryoris @rahaman-quantum
C-API: Add fix for Windows and C++ (Qiskit#14273)
54cdf0a 
* Add fix for Windows and C++

* added ifdef for C++

* fix Makefile for Windows

* fix Makefile

* use std::complex for C++

* add include <complex> for C++, set OS if not Windows

* directly define lib filename

* Update releasenotes/notes/fix-c-api-for-windows-and-cpp-0b8d79cd3fcf975a.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* fix code block annotation

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cryoris Cryoris Cryoris approved these changes

Assignees
No one assigned
Labels
C API Related to the C API Changelog: Bugfix Include in the "Fixed" section of the changelog Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@doichanj @qiskit-bot @coveralls @Cryoris @mtreinish