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

C++ compatibility errors with stringzilla.h #84

Closed
WillisMedwell opened this issue Feb 7, 2024 · 5 comments
Closed

C++ compatibility errors with stringzilla.h #84

WillisMedwell opened this issue Feb 7, 2024 · 5 comments
Labels
cpp good first issue Good for newcomers

Comments

@WillisMedwell
Copy link
Contributor

WillisMedwell commented Feb 7, 2024

Using clang-cl this error is generated when including stringzilla.hpp.

C:/dev/utily/build-native/_deps/stringzilla-src/include\stringzilla/stringzilla.h:1946:50: warning: use of old-style cast [-Wold-style-cast]
    if (h_length < n_length || !n_length) return SZ_NULL;
                                                 ^~~~~~~
C:/dev/utily/build-native/_deps/stringzilla-src/include\stringzilla/stringzilla.h:1095:18: note: expanded from macro 'SZ_NULL'
#define SZ_NULL ((void *)0)
                 ^       ~
C:/dev/utily/build-native/_deps/stringzilla-src/include\stringzilla/stringzilla.h:1946:50: error: cannot initialize return object of type 'sz_cptr_t' (aka 'const char *') with an rvalue of type 'void *'
    if (h_length < n_length || !n_length) return SZ_NULL;
                                                 ^~~~~~~
C:/dev/utily/build-native/_deps/stringzilla-src/include\stringzilla/stringzilla.h:1095:17: note: expanded from macro 'SZ_NULL'
#define SZ_NULL ((void *)0)

To resolve this issue, I think you can wrap the include in file: stringzilla.hpp line 62:
From

#include <stringzilla/stringzilla.h>

To

extern "C" {
     #include <stringzilla/stringzilla.h>
}

But I'm not 100%, probably worth some investigating.

In addition, fetch content section probably isn't verbose enough (especially considering the joy of working with cmake)

   # without this fetchcontent will fail with no indicative error message
   include(FetchContent)

    FetchContent_Declare(
        stringzilla 
        GIT_REPOSITORY https://github.com/ashvardanian/stringzilla.git 
        GIT_TAG main     # Need this or the operation fails
        GIT_SHALLOW FALSE
    )
    FetchContent_MakeAvailable(stringzilla)

    # Not sure how you intend for users to link/find your library as make_available 
    # doesn't add it to include path or anything. So, you have to do something like this.
    target_link_libraries(my_proj PRIVATE stringzilla)
@ashvardanian
Copy link
Owner

Thanks for suggestions, @WillisMedwell! Both sound reasonable! Can you please open a PR into main-dev?

@ashvardanian ashvardanian added good first issue Good for newcomers cpp labels Feb 8, 2024
@WillisMedwell
Copy link
Contributor Author

It's a bit difficult to debug when there are no example projects to validate the changes, would you consider adding an examples folder with different language projects? Due to the many supported languages, it seems difficult at the moment to validate each one.

You could then run an action to validate each of them, and it also provides users with examples of how to integrate with their python, c++, and other projects.

I would be happy to do this for C++ if its something that aligns with your vision of the repo?

@ashvardanian
Copy link
Owner

Not sure what you mean, @WillisMedwell. Have you checked the CONTRIBUTING.md and the scripts folder? In VSCode you will also get tasks and launchers preconfigured once you open the project.

@WillisMedwell
Copy link
Contributor Author

@ashvardanian my bad i didnt realise the testing suite was under ./scripts.

But my point being that you could have an ./example folder with different ways of installing the lib for c++, python ect. Then you could validate, demonstrate & scope the intended way of using the lib. I only suggest this as 'zilla supports many different languages and it might be difficult to know when stuff breaks.

@ashvardanian
Copy link
Owner

@WillisMedwell, there is a CI pipeline on GitHub. We don't merge until the tests for all languages pass and environments pass.

Regarding a separate folder for examples, I try to avoid branching the directory tree too much. Cause then I'll have to make separate folders for unit tests, fuzzy tests, stress tests, benchmarks... across all languages.

Hope this makes sense 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants