-
-
Notifications
You must be signed in to change notification settings - Fork 49
Make normalization optional to fix zsl #56
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
Conversation
I think it's better to keep C++ signatures so that it feels natural for users that would like to call the library from C++ code, however we can use preprocessor directives to only expose C-compatible functions when used in C so that we can rename them to have the same names in both cases. Meaning we go from
to
EDIT: A con of this is that it would require doing |
I think functions with the same name with different signatures would make it too confusing. I'd prefer to either keep both versions or expose only C-compatible functions without |
Yeah, that's a valid concern.
In that case, I would recommend making the entire public API in I can open a PR to take a stab at this if that's fine with you. |
Yeah, this is what I'm also thinking of.
That would be awesome. You can target this PR (or directly push to it) and then we can merge to the main branch at once. Thanks for the colaboration! |
* main.c: remove hardcoded vec_dim * Make all public functions use C-compatible signatures * Move private declarations to clip.cpp * Merge with fix-zsl * Update examples * python: rm redundant bindings to CLIPLayer and models * formatting: apply formatting script * readme: add notices about the new API * zsl: use const auto & * tests: fix benchmark utility * python: bump version * Add constructor-like functions for batched structs --------- Co-authored-by: M. Yusuf Sarıgöz <[email protected]>
closes #54
Since we are already changing function signatures, this might be a good opportunity to use C-compatible signatures everywhere and remove functions with
_c
suffix. WDYT?