Skip to content

feat: wrapper API's using subranges#5

Merged
chaitanyaprem merged 8 commits intomasterfrom
feat/subrange-wrapper
May 1, 2024
Merged

feat: wrapper API's using subranges#5
chaitanyaprem merged 8 commits intomasterfrom
feat/subrange-wrapper

Conversation

@chaitanyaprem
Copy link
Copy Markdown
Collaborator

Lot of code duplication as of now, since i had just copied the functions using storage type.
This will be useful to test functionality first.

Will refactor in a separate PR.

Copy link
Copy Markdown

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Can't really review C/C++ code but maybe @NagyZoltanPeter could?

@chaitanyaprem
Copy link
Copy Markdown
Collaborator Author

Can't really review C/C++ code but maybe @NagyZoltanPeter could?

Will add him.

Anyways, there is nothing special but a lot of copying :)

Once optimizations are done, then i would want thorough review though.

@NagyZoltanPeter
Copy link
Copy Markdown

Can't really review C/C++ code but maybe @NagyZoltanPeter could?

Will add him.

Anyways, there is nothing special but a lot of copying :)

Once optimizations are done, then i would want thorough review though.

Ok, I can review it no problem. I will do it in the evening.

Copy link
Copy Markdown

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

I'm just adding some comments for now that I hope you find useful :)

Comment thread cpp/example/test.c
Comment thread cpp/example/test.c Outdated
Comment thread cpp/example/test.c Outdated
Comment thread cpp/negentropy_wrapper.c
Comment thread cpp/negentropy_wrapper.c
Comment thread cpp/negentropy_wrapper.c
Copy link
Copy Markdown

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Overall it is working obviously.
I agree to tackle error handling in a separate PR.
In general I'm not a big fan of returning c++ objects as void* and communicate with the lib via them. As soon as we can keep it in-house it can be ok and effective. But given as an open source stuff it is not advisable to use by third parties.
One generic comment: I would not call negentropy_wrapper.c but .cxx/.cpp/.cc but not .c as that one is a c++ source file.
There is a new std::string in the code which should not be heap allocated, stack string is simple ok. (Not changed in this PR).
Error handling would be nice if we define error codes and const error text for the interface.

@chaitanyaprem
Copy link
Copy Markdown
Collaborator Author

There is a new std::string in the code which should not be heap allocated, stack string is simple ok. (Not changed in this PR).

Thanks for pointing this out! Initially this was an out argument which i later modified to use result. This was remnant which has to be fixed.

@chaitanyaprem
Copy link
Copy Markdown
Collaborator Author

One generic comment: I would not call negentropy_wrapper.c but .cxx/.cpp/.cc but not .c as that one is a c++ source file.

Agreed. It was named as C file as we were writing a C wrapper for the C++ code but that is anyways addressed in header file via EXTERNC. We can rename it into cpp file.

@chaitanyaprem
Copy link
Copy Markdown
Collaborator Author

Error handling would be nice if we define error codes and const error text for the interface.

Any suggestions of how to handle it from C to nim?
Many of these functions already return pointer to something, one way is simply wrap the pointer and error into another struct similar to what i did for result. But that doesn't seem very generic approach, if you have any thoughts please suggest.

@chaitanyaprem chaitanyaprem merged commit 311a21a into master May 1, 2024
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.

4 participants