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 c api bindings for lgraph #180

Merged
merged 16 commits into from
Jun 16, 2023

Conversation

antkiller996
Copy link
Contributor

@antkiller996 antkiller996 commented Mar 25, 2023

Here are c api bindings for lgraph.

The implementation is really intuitive.

  • Every C++ class within lgraph_api namespace is ported to an opaque c struct struct lgraph_api_cxx_class_name_t.
  • All public constructors are ported to c functions with signature like lgraph_api_cxx_class_name_t* lgraph_api_cxx_class_name_create(param1, param2, ...)
  • All destructors are ported to c functions with signature like void lgraph_api_cxx_class_name_destroy(lgraph_api_cxx_class_name_t*)
  • All public methods are ported to c functions with signature like return_type lgraph_api_cxx_class_name_method_name(param1, param2, ...). If there are multiple overloaded variants, there should be like return_type lgraph_api_cxx_class_name_method_name_{by, with}_param1(param1, param2, ...)
  • C++ exceptions are ported to c string storing the error message returned from e.what()

@antkiller996 antkiller996 marked this pull request as draft March 25, 2023 13:21
@antkiller996 antkiller996 force-pushed the feat-cbindings branch 2 times, most recently from f1e4c63 to 69ca7b7 Compare May 16, 2023 07:07
@CLAassistant
Copy link

CLAassistant commented May 16, 2023

CLA assistant check
All committers have signed the CLA.

@antkiller996 antkiller996 force-pushed the feat-cbindings branch 2 times, most recently from 1c5dc4a to 9e2c254 Compare May 16, 2023 09:05
@antkiller996 antkiller996 marked this pull request as ready for review June 14, 2023 05:46
@antkiller996 antkiller996 changed the title WIP: add c bindings of lgraph api add c bindings of lgraph api Jun 14, 2023
@antkiller996 antkiller996 changed the title add c bindings of lgraph api add c bindings for lgraph api Jun 14, 2023
@antkiller996 antkiller996 changed the title add c bindings for lgraph api add c api bindings for lgraph Jun 14, 2023
include/lgraph/c.h Show resolved Hide resolved
src/c/c.cpp Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
test/test_c.cpp Show resolved Hide resolved
@@ -0,0 +1,3 @@
add_library(lgraph-cbindings STATIC c.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

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

The first thing to remember is that the declaration of lgraph_api is in include directory, and its definition is in src/lgraph_api directory.

Logically c-bindings is part of lgraph_api, now the problem is that C API has been added into include, but its definition is not packed in ${TARGET_LGRAPH}. Please fix this in BuildLGraphApi.cmake.

src/c/c.cpp Outdated
@@ -0,0 +1,2835 @@
/**
* Copyright 2022 AntGroup CO., Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the implementation directory, it is recommended to rearrange this to src/lgraph_api/c.cpp.

@@ -9,6 +9,7 @@ include(BuildLGraphServer.cmake)
include(BuildCypherLib.cmake)
include(BuildClients.cmake)
include(BuildCythonExtension.cmake)
add_subdirectory(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend merging the cmakelists file of build-c-bindings into BuildLGraphApi.cmake, since c-bindings should be part of lgraph_api

@antkiller996
Copy link
Contributor Author

Now, c biindings are packed into lgraph_api target and src/c/c.cpp is moved to src/lgraph_api/c.cpp. @wangtao9

Options.cmake Outdated Show resolved Hide resolved
src/BuildLGraphApi.cmake Outdated Show resolved Hide resolved
@wangtao9 wangtao9 merged commit b8dcaac into TuGraph-family:master Jun 16, 2023
2 checks passed
@antkiller996 antkiller996 deleted the feat-cbindings branch June 20, 2023 04:51
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.

None yet

3 participants