-
-
Notifications
You must be signed in to change notification settings - Fork 32
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 wrapper to basix for tabulation in C-code #612
Conversation
…laus/custom-integrals
cpp/CMakeLists.txt
Outdated
@@ -1,7 +1,7 @@ | |||
cmake_minimum_required(VERSION 3.16) | |||
|
|||
# Set the version | |||
project(Basix VERSION "0.5.2.0" LANGUAGES CXX) | |||
project(Basix VERSION "0.5.2.0" LANGUAGES CXX C) |
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.
Is this necessary? I'm thinking the build for Basix will still be CXX. The downstream project would use C.
/// enum classes in basix | ||
typedef struct basix_element basix_element; | ||
|
||
basix_element* basix_element_create(const int basix_family, const int basix_cell_type, const int degree, |
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.
It's super tedious, but I think to make this interface useable you will need to recreate the C++ enums as C enums.
@@ -0,0 +1,61 @@ | |||
// Copyright (c) 2022 Susanne Claus |
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.
We use clang-format on our C++ files.
basix::element::family family = static_cast<basix::element::family>(basix_family); | ||
basix::cell::type cell_type = static_cast<basix::cell::type>(basix_cell_type); | ||
int k = degree; | ||
basix::element::lagrange_variant lvariant = static_cast<basix::element::lagrange_variant>(lagrange_variant); |
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.
lvariant
etc. should also be const
.
|
||
extern "C" { | ||
|
||
basix_element* basix_element_create(const int basix_family, const int basix_cell_type, const int degree, |
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 think using int and then casting to the basix enum is OK, but I think we should explicitly add the int datatype to the class enums in basix, e.g.:
https://github.com/FEniCS/basix/blob/main/cpp/basix/element-families.h
https://en.cppreference.com/w/cpp/language/enum
"Values of integer, floating-point, and enumeration types can be converted by static_cast or explicit cast, to any enumeration type. If the underlying type is not fixed and the source value is out of range, the behavior is undefined."
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.
A good alternative would be to have C enum equivalents of the existing C++ enums, as mentioned above.
@@ -0,0 +1,43 @@ | |||
// Copyright (c) 2022 Susanne Claus |
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.
Could you add an example of use in e.g. demo/c/wrapper
?
That's often helpful for getting the API looking right.
{ | ||
//Specify which element is needed | ||
basix::element::family family = static_cast<basix::element::family>(basix_family); | ||
basix::cell::type cell_type = static_cast<basix::cell::type>(basix_cell_type); |
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.
reinterpret_cast
might work?
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.
Possibly not a good idea
I will fix the SonarCloud error for you on another branch. |
No activity for some time, so closing. |
C-wrapper for basix functions tabulate_shape and tabulate.
This C-wrapper is used to evaluate basis functions and their derivatives at a given set of points in generated C-code. This is needed for run-time quadrature evaluation used in methods such as CutFEM. This pull request contains the C-Wrapper as well as some small modifications of CMakeLists.txt in basix and is linked to #602 . @mscroggs @chrisrichardson