Fix: repetitive visibility warnings, -Werror. #61
Conversation
|
Ah, ok, -Werr exposed some stuff. Hang on ... |
|
Ah, perhaps not. This seems to be a real issue: python/lib.c: In function ‘Str_levenshtein’: |
|
Not sure how that ever compiled, actually? The non-existent user_data member ref came in at change 44ef989 along with the definition of sz_memory_allocator_t. |
include/stringzilla/stringzilla.h
Outdated
| @@ -181,6 +175,9 @@ | |||
| #endif | |||
| #endif | |||
|
|
|||
| #include <stdio.h> | |||
There was a problem hiding this comment.
The goal of this header is to be entirely independent from LibC, so we have to remove those includes.
scripts/search_test.py
Outdated
| a = get_random_string(length=20) | ||
| b = get_random_string(length=20) | ||
| assert sz.levenshtein(a, b, 200) == levenshtein(a, b) | ||
| @pytest.mark.parametrize("reps", [1]) |
There was a problem hiding this comment.
There is a pytest-repeat extension, that adds the decorator. The canonical way would be to use it, and also reset the seed in the main body of the test on every invocation.
There was a problem hiding this comment.
What's your opinion on typedef struct in stringzilla.h for sz_u64_vec_t and similar constructs. Some C developers are against them, assuming the global namespace gets polluted, but the current implementation doesn't use too many structures and should probably be fine.
src/avx2.c
Outdated
| uint32_t u16s[sizeof(uint64_t) / sizeof(uint32_t)]; | ||
| uint16_t u32s[sizeof(uint64_t) / sizeof(uint16_t)]; | ||
| uint8_t u8s[sizeof(uint64_t) / sizeof(uint8_t)]; | ||
| } u64_parts_t; |
There was a problem hiding this comment.
The src/ folder was a temporary decision during the v3 preparation. I'll remove it in a bit to avoid confusion. Sorry, I got a bit used to working on this alone :)
Testing: built and ran stringzilla_search_{bench,test}
Lots of warnings about the symbol visibility directives. These directives would make sense if stringzilla were distributed primarily as a .so or .dylib, but we appear to expose it mostly as a header file. Not sure how this impacts python or js, but also unable to build these at the time so consider this a starting point.