-
Notifications
You must be signed in to change notification settings - Fork 504
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 minimal c++ example and check #1944
Add minimal c++ example and check #1944
Conversation
4631289
to
4b96325
Compare
@aidenfoxivey We have a header-only C++ wrapper that provides all the required C++ RAII for liboqs, https://github.com/open-quantum-safe/liboqs-cpp |
@vsoftco Would it be fair to rework this with the C++ bindings and then continue working on the PR? |
Absolutely fine to close the PR too, but I feel like it should probably be addressed in the issue first, just to make the procedure clear. |
@vsoftco The purpose of this is to catch syntax errors in C++. For example when the stateful-sigs PR was in progress we almost merged something that compiled for C but failed to link against a C++ target. Since we have |
@Martyrshot Do we want to compile and run the resulting example file? (and/or use valgrind to check it for leaks?) in my existing implementation I'm just compiling and linking it. |
I think valgrind for this is overkill, but running it is a good idea. It could catch weird runtime linking errors. Sorry I should have thought about this sooner. Would it be much work for you to add this test to the ninja If it's too much work, I can take a look into doing this later in the week. |
Oh no, don't worry about it! I can absolutely sort it out later tonight or tomorrow. |
@Martyrshot I thought about it a bit and, if I understand you correctly, this requires modifying the CMakeLists.txt to add C++ as a LANGUAGE. Are you cool with that? |
Would we be able to isolate that change to just this test? |
The |
That's fair. Then in that case I think we should avoid modifying |
Should have this sorted by end of next week. Apologies about the delay - midterms :) |
Good luck! |
No worries! Good luck! |
ac2c329
to
90030a4
Compare
Adding a simple C++ test to CI.
Fixes #1770.
Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
Doesn't change any cryptographic algorithms.
Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)
Doesn't change any cryptographic algorithms.