Do not set feature test macro in public header #320
Conversation
horgh
force-pushed
the
horgh/posix
branch
2 times, most recently
from
f51c9c9
to
026efa9
Compare
We believe that this should be set by applications rather than by the library. The spec suggests that having it in this spot is not correct as it should be set prior to any header being included. See https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02. Fixes #318
There was a problem hiding this comment.
One optional improvement. This or even an internal header throughout might be a bit cleaner, but I don't have strong opinion.
Do you think we will need to update any of our extensions for this change? If so, we should be sure to release those before we release this.
t/bad_databases_t.c
Outdated
| @@ -1,5 +1,8 @@ | |||
| // This test currently does not work on Windows as nftw is | |||
| // not available. | |||
| #ifndef _POSIX_C_SOURCE | |||
| #define _POSIX_C_SOURCE 200809L | |||
| #endif | |||
There was a problem hiding this comment.
It doesn't matter too much, but could we have just set these in maxminddb_test_helper.h?
There was a problem hiding this comment.
I think we could. The reason I didn't is the spec says we should be setting them prior to including any header. I suppose it is fine to do that though given it's test code!
|
I tested the Python, PHP, and Perl extensions with this version of libmaxminddb installed. I ran the tests as shown in the GitHub test action for each. All ran without issue, so I think we're okay with the extensions as they are. |
|
Console output of the tests - https://gist.github.com/horgh/f6c8b7a5ba3b042fcccb4111481bf214 |
No description provided.