christophercrouzet / rexo

Neat single-file cross-platform unit testing framework for C/C++.
The Unlicense
27 stars 6 forks source link

[include/rexo.h] Fix `warning: implicitly declaring library function … #4

Closed SamuelMarks closed 4 years ago

SamuelMarks commented 4 years ago

…'vsnprintf'`

Also not sure if we want to hack away this:

#if !defined(__cplusplus) && __STDC_VERSION__ >= 199901L
    typedef unsigned long long rx_uint64;
#else
    typedef unsigned long rx_uint64;
#endif

So that it actually works on C89 over: https://github.com/christophercrouzet/rexo/blob/6e2669d/include/rexo.h#L68

christophercrouzet commented 4 years ago

Hello @SamuelMarks, and thanks for submitting this merge request!

I am unfamiliar with this issue so, if that's OK, I would love to understand what's going on before accepting the merge.

After a bit of Googling, I'm assuming that this is somewhat related to macOS, is this correct?

Also, this code would mostly be called when an assertion fails and that the user would provide a custom error message, which seems to be covered by the tests (see assertion-failure-messages.c), however the test suite doesn't seem to pickup any issue even when ran in macOS Catalina (see test.yml).

Would you happen to have a repro that I could run (hopefully without me needing to install macOS :stuck_out_tongue:)?

Cheers!

SamuelMarks commented 4 years ago

@christophercrouzet So there's the macOS side, then there's the including it in my CMakeLists.txt side…

Basically I just include rexo.h and set(CMAKE_C_STANDARD 90) and

    add_compile_options(
            "$<$<CONFIG:DEBUG>:-Wall>"
            "$<$<CONFIG:DEBUG>:-Werror>"
            "$<$<CONFIG:DEBUG>:-pedantic>"
            "$<$<CONFIG:DEBUG>:-Wno-missing-braces>"
            "$<$<CONFIG:RELEASE>:-O3>"
    )

    if (CMAKE_SYSTEM_NAME STREQUAL "Linux")
        add_compile_options("$<$<CONFIG:DEBUG>:-pedantic>")
    else ()
        add_compile_options("$<$<CONFIG:DEBUG>:-Wpedantic>")
    endif ()
christophercrouzet commented 4 years ago

Some of the tests are already running with CMAKE_C_STANDARD 90, as shown in the CMakeLists.txt file.

Which version of macOS are you using? Which compiler (and its version)?

christophercrouzet commented 4 years ago

Also, if you have the chance, could you please execute the following code and paste its output here?

#include <stdio.h>
#include <rexo.h>

int
main(void)
{
#if defined(__cplusplus)
    printf("__cplusplus      : 1\n");
#else
    printf("__cplusplus      : 0\n");
#endif

    printf("RX__LANG         : %d\n", RX__LANG);
    printf("RX__LANG_VERSION : %ld\n", RX__LANG_VERSION);
    printf("RX__HAS_NPRINTF  : %d\n", RX__HAS_NPRINTF);

    return 0;
}
SamuelMarks commented 4 years ago

Sure, here's a version without CMake, with main.c being your aforementioned code:

$ curl -OL https://raw.githubusercontent.com/christophercrouzet/rexo/master/include/rexo.h
$ mkdir include && mv rexo.h $_
$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 11.0.3 (clang-1103.0.32.62)
Target: x86_64-apple-darwin19.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
$ gcc main.c -Iinclude -std=c90 -Wall -Werror -pedantic -Wpedantic -O3
In file included from main.c:2:
include/rexo.h:68:22: error: 'long long' is an extension when C99 mode is not enabled [-Werror,-Wlong-long]
    typedef unsigned long long rx_uint64;
                     ^
1 error generated.
$ ./a.out
__cplusplus      : 0
RX__LANG         : 0
RX__LANG_VERSION : 0
RX__HAS_NPRINTF  : 0
christophercrouzet commented 4 years ago

Awesome, thank you @SamuelMarks!

Where I'm getting at with this is: vsnprintf (and long long) was only added to the C standard since C99. My approach with Rexo has been to remain standard-compliant as much as possible, which means not using vsnprintf for C89 and providing a fallback instead.

This fallback is implemented here.

As you can see, this fallback should be picked when RX__HAS_NPRINTF evaluates to 0, which seems to be the case from your log output, so I do not understand how your changes help with this issue since vsnprintf shouldn't be used in your case?

Furthermore, the preprocessor condition

 #if RX__HAS_NPRINTF && defined(__STDC__) && (!defined(__STDC_VERSION__) || __STDC_VERSION__ <= 199901L)

is a bit contradictory—if you look at the definition of RX__HAS_NPRINTF here, then you'll see that RX__HAS_NPRINTF should be set to 0 when __STDC_VERSION__ < 199901L is true.

So my question is: would it be possible that you defined the macro RX_ENABLE_NPRINTF to have Rexo forcefully use vsnprintf?

Regarding the rx_uint64 issue, I've been wanting to address it but I haven't found a good solution yet. I'd really like to ensure that it's always 64-bit, even in C89, since it's the precision required to handle the results given by the timing APIs from the various platforms.

christophercrouzet commented 4 years ago

I'll be closing this pull request for now but please feel free to reopen it if you'd like to further discuss the issue!