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

Fix a handful of compilation issues with older C standards #480

Merged
merged 5 commits into from
May 23, 2023

Conversation

ttaylorr
Copy link
Contributor

This pull request fixes a small handful of compilation issues noticed when integrating CRoaring into the Git project and compiling it with DEVELOPER=1 enabled.

When compiling without Git's strict checks, everything works as expected (here and elsewhere I'm using the amalgamation build):

$ make croaring/roaring.o
    * new build flags
    CC croaring/roaring.o

However, Git cannot compile the amalgamated build when DEVELOPER=1 is set. Setting DEVELOPER=1 when building enables a handful of stricter compilation checks, including -Werror=implicit-function-declaration, -Werror=old-style-definition, -Werror=missing-prototypes. Compiling CRoaring with those flags does not currently work, but does (at least on my system!) after this PR.

(For posterity: Git's DEVELOPER=1 mode also sets -Wdeclaration-after-statement, which produces ~300-ish warnings when compiling CRoaring. CRoaring and Git differ in convention here, and there isn't a need to force either project to change here, since we can #pragma ignore these).

ttaylorr added 5 commits May 22, 2023 16:48
The assertion added via 264b188 (Using atomic counters on shared
containers (RoaringBitmap#473), 2023-05-15) does not explicitly include <assert.h>
causing GCC to fail compilation when invoked with
`-Werror=implicit-function-declaration`.

In C++ this is OK, since there `assert()` refers to its macro
definition. But in C the only way to get `assert()` is by including
<assert.h>.

Signed-off-by: Taylor Blau <[email protected]>
A handful of functions with argument arity zero fail when the compiler
is invoked with `-Werror=old-style-definition`. Work around these by
explicitly declaring the parameter list as `void` to clarify that these
functions expect zero arguments.

Signed-off-by: Taylor Blau <[email protected]>
This function dates all the way back to 3d12719 (some more untested
code, plus some files that had not been tracked hitherto, 2016-01-19),
but never had a prototype.

This causes compilers building with `-Werror=missing-prototypes` to fail
compilation. Declare a prototype for that function in the corresponding
header accordingly.

Signed-off-by: Taylor Blau <[email protected]>
In a similar spirit as the previous commit, declare a function prototype
for `bitset_container_union_nocard()` which rounds out the existing set
of function prototypes for the macro expansion of:

    BITSET_CONTAINER_FN(union, |, _mm256_or_si256, vorrq_u64)

Signed-off-by: Taylor Blau <[email protected]>
In a similar spirit as the previous commits, declare a function
prototype for `bitset_container_intersection_nocard()` which rounds out
the existing set of function prototypes for the macro expansion of:

    BITSET_CONTAINER_FN(intersection, &, _mm256_and_si256, vandq_u64)

Signed-off-by: Taylor Blau <[email protected]>
@lemire
Copy link
Member

lemire commented May 23, 2023

We shall merge once tests pass. I see that @Dr-Emann has approved, so we are good.

@lemire
Copy link
Member

lemire commented May 23, 2023

(For posterity: Git's DEVELOPER=1 mode also sets -Wdeclaration-after-statement, which produces ~300-ish warnings when compiling CRoaring. CRoaring and Git differ in convention here, and there isn't a need to force either project to change here, since we can #pragma ignore these).

If someone wants to do the leg work of cleaning this out, we would not reject the PR.

@ttaylorr
Copy link
Contributor Author

If someone wants to do the leg work of cleaning this out, we would not reject the PR.

Good to know. I think for now my plan is to just ignore these with:

#pragma GCC diagnostic ignored "-Wdeclaration-after-statement"

But it would be nice to clean them out if you're open to it. I personally find putting all of the declarations at the beginning of a function is much more readable, but not sure if that opinion is widely held.

@lemire
Copy link
Member

lemire commented May 23, 2023

I personally find putting all of the declarations at the beginning of a function is much more readable, but not sure if that opinion is widely held.

Personally, I tend not to care. However, if we change it, we need to have tests in CI so that we don't revert.

@lemire lemire merged commit 6db7fbc into RoaringBitmap:master May 23, 2023
@ttaylorr ttaylorr deleted the ttaylorr/git branch May 23, 2023 02:43
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.

3 participants