-
Notifications
You must be signed in to change notification settings - Fork 36
Add c++20 kernel #77
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 c++20 kernel #77
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 51.60% 52.42% +0.81%
==========================================
Files 15 15
Lines 593 599 +6
==========================================
+ Hits 306 314 +8
+ Misses 287 285 -2 |
CMakeLists.txt
Outdated
@@ -154,6 +154,8 @@ configure_kernel("share/jupyter/kernels/xcpp") | |||
configure_kernel("share/jupyter/kernels/xcpp11") | |||
configure_kernel("share/jupyter/kernels/xcpp14") | |||
configure_kernel("share/jupyter/kernels/xcpp17") | |||
configure_kernel("share/jupyter/kernels/xcpp17-omp") |
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.
I am not sure I understand how this works. We need to pass somehow -fopenmp
as a flag. XEUS_CPP_OMP
probably does not expand to anything...
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.
I'll remove adding the omp-17 kernel and move that to a different PR. I assumed that infrastructure was there to use it, but had accidentally been missed. I'll leave this PR to just adding the c++20 kernel.
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.
I'll remove adding the omp-17 kernel and move that to a different PR. I assumed that infrastructure was there to use it, but had accidentally been missed. I'll leave this PR to just adding the c++20 kernel.
@vgvassilev I've made this change now, so the PR should hopefully be ok to merge now.
d1b646c
to
dd506c4
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.
LGTM. I'd wait for @JohanMabille.
The purpose of this PR is to add a c++20 kernel.