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: introduce init functions to avoid warnings. #631

Closed
wants to merge 2 commits into from

Conversation

lemire
Copy link
Member

@lemire lemire commented Jun 3, 2024

It is difficult to portably initialize structs to zero without a function. It turns out that either C or C++ emits a warning.

@madscientist
Copy link
Contributor

It's interesting, I don't think you'll get warnings if you compile your C code (like art.c) with a C compiler not a C++ compiler. The structure = {0} is legal C code since C99. Is there a reason to want to use a C++ compiler to compile art.c etc.? The initializations are not in header files so they aren't visible to both compilers AFAICT.

@lemire
Copy link
Member Author

lemire commented Jun 4, 2024

Is there a reason to want to use a C++ compiler

We explicitly support C++ compilation as a you can see by looking at the code: (see screenshot)

image

We have been supporting both C and C++ compilation of the source code for quite some time now.

The issue here is to get fewer warnings at compile time.

@lemire
Copy link
Member Author

lemire commented Jun 4, 2024

The initializations are not in header files so they aren't visible to both compilers AFAICT.

When compiling with C++, you compile to a namespace. So you have to take your pick: C or C++. But if you pick C++, you must build everything in C++.

@@ -40,6 +40,10 @@ namespace roaring {
namespace internal {
#endif

void art_iterator_init(art_iterator_t *iterator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/style: I suggest moving this to line 123 or somewhere around there, so all typedefs are at the top before utility functions.

@@ -21,8 +21,9 @@ namespace roaring {
namespace api {
#endif

// TODO: Copy on write.
// TODO: Error on failed allocation.
void roaring64_bulk_context_init(roaring64_bulk_context_t *context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/style: Same here, suggest moving it below the typedefs

@madscientist
Copy link
Contributor

OK. If it were me I'd probably forgo the extra function and just make use of the preprocessor instead:

#if __cplusplus
# define CROARING_INIT {}
#else
# define CROARING_INIT {0}
#endif
   ...
    art_iterator_t iterator = CROARING_INIT;

but this works as well.

@lemire
Copy link
Member Author

lemire commented Jun 5, 2024

@madscientist We can try that.

@lemire
Copy link
Member Author

lemire commented Jun 5, 2024

See #634

@lemire
Copy link
Member Author

lemire commented Jun 5, 2024

Closing.

@lemire lemire closed this Jun 5, 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.

3 participants