roaring/portability.h leads to build errors on MacOS when used with other libraries

[Taepper]

I detect this error on roaring version 4.2.1, but also in older versions.

roaring/portability.h contains the following code:

#if !(defined(_XOPEN_SOURCE)) || (_XOPEN_SOURCE < 700)
#define _XOPEN_SOURCE 700
#endif // !(defined(_XOPEN_SOURCE)) || (_XOPEN_SOURCE < 700)

This leads to problems on MacOS when including both header files from roaring and also including header files from other libraries. In my case this happens when including roaring before asio.

The following code simple test program leads to errors:

#include <roaring/roaring.hh>

#include <asio.hpp>

int main() {}

Even undefining _XOPEN_SOURCE does not remedy the situation:

#include <roaring/roaring.hh>
#undef _XOPEN_SOURCE
#include <asio.hpp>

int main() {}

as roaring already included system header files (in this case <netdb.h>, <netinet/in.h>), whose definitions are expected by asio.hpp.

This fixes this particular instance:

#include <netdb.h>
#include <netinet/in.h>

#include <roaring/roaring.hh>

#include <asio.hpp>

int main() {}

To circumvent such issues in the future, maybe the portability header should only be included in either test builds or in object files? I am happy to contribute changes

Here are some further links describing similar issues and discussions thereof:
kcat/openal-soft#4
https://lwn.net/Articles/540831/

[Taepper]

.. another data point that might be of interest:

Defining _DARWIN_C_SOURCE also fixes this issue:

#define _DARWIN_C_SOURCE

#include <roaring/roaring.hh>

#include <asio.hpp>

int main() {}

[lemire]

To circumvent such issues in the future, maybe the portability header should only be included in either test builds or in object files? I am happy to contribute changes

Do you think that it is feasible? If so, that would be fantastic…

[Taepper]

After looking into this a little bit, can I ask what the original reason was for defining the _XOPEN_SOURCE preprocessor macro?

As portability.h contains many declarations that are definitely needed in header files of this project, I think one should remove this flag from portability.h.

I currently see two options when it is still desired:

1. pass it via the build system in the cases where it should be enabled
2. define it in another file that is guaranteed to be included first in all compilation units it is used in. (i.e. only use it in object files of this repository, as any downstream user might include other stuff before including roaring.h)

[Taepper]

Note the following guidance (also for the _GNU_SOURCE preprocessor macro you removed in #693)

You should define these macros by using ‘#define’ preprocessor directives at the top of your source code files. These directives must come before any #include of a system header file. It is best to make them the very first thing in the file, preceded only by comments. You could also use the ‘-D’ option to GCC, but it’s better if you make the source files indicate their own meaning in a self-contained way.

https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html

[lemire]

I have no idea what these macros do. I have tried removing the xopen source thing here: #695

[Taepper]

Great! Thank you!

Then it could be noted that the definition of _POSIX_C_SOURCE in the same file also violates the gnu manual:

These directives must come before any #include of a system header file

However, this has not caused me any errors and I am not sure whether this enables necessary features from system headers.

[lemire]

@Taepper Can you explain how we violate the rule that you quote? I don't see it.

[Taepper]

roaring/portability.h contains the following lines:

#if defined(_POSIX_C_SOURCE) && (_POSIX_C_SOURCE < 200809L)
#undef _POSIX_C_SOURCE
#endif

#ifndef _POSIX_C_SOURCE
#define _POSIX_C_SOURCE 200809L
#endif  // !(defined(_POSIX_C_SOURCE)) || (_POSIX_C_SOURCE < 200809L)

which always defines _POSIX_C_SOURCE.

_POSIX_C_SOURCE is a GNU feature test macro, documented here:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html

The manual specifies, that:

These directives must come before any #include of a system header file. It is best to make them the very first thing in the file, preceded only by comments. You could also use the ‘-D’ option to GCC, but it’s better if you make the source files indicate their own meaning in a self-contained way.

While the macro is the first thing in the file roaring/portability.h, a downstream user may include other stuff before including roaring.h, which will indirectly includes this file. Therefore, it generally is not the first thing in any given compilation unit that includes roaring/portability.h, violating the first sentence.

Take this file as an example: https://github.com/RoaringBitmap/CRoaring/blob/master/include/roaring/containers/array.h

It will define _POSIX_C_SOURCE only after the following includes:

#include <string.h>

#include <roaring/roaring_types.h>  // roaring_iterator

// Include other headers after roaring_types.h
#include <roaring/array_util.h>  // binarySearch()/memequals() for inlining
#include <roaring/containers/container_defs.h>  // container_t, perfparameters