skeeto / pdjson

C JSON parser library that doesn't suck
The Unlicense
279 stars 36 forks source link

few questions #12

Closed aleks-f closed 6 years ago

aleks-f commented 6 years ago

@skeeto

1) what is json_typename ? I don't see it used anywhere and it breaks some older MSVC builds, can it be removed?

2) would you mind few explicit typecasts, so pdjson.c can be compiled as C++? I know it is superfluous for C, but we are forced to compile as C++ for pre-VS2013 builds because C99 support is dismal there

3) this

#include <stdio.h>
#include <stdbool.h>

is in both source and header. can we get rid of the source file entries?

If you're ok with the above, I'll send pull

skeeto commented 6 years ago

I'm using it in two different test programs, so I moved it to the library. It's exposed in the header file as part of the API:

https://github.com/skeeto/pdjson/blob/b569a7c9d88386f8a129431a544c5bbef7e4ce37/pdjson.h#L28

I had forgotten about MSVC's lack of C99 features like designated array initializers, as well as its weird issues with linking data symbols. So I just moved it out to each of the tests (92cca21). No big deal.

I suppose having more explicit typecasts is alright. I went ahead and did the deed myself (da3b34f) since I wanted to understand the damage first-hand. The code should all now be conforming C++98, except for that variadic macro which is technically only valid after C++11.

aleks-f commented 6 years ago

great, super fast - thank you!

if you don't mind removing the duplicate includes from source, that would be great, too - since we still have to patch that (no stdbool.h on old MSVC, sigh), it's easier if it's only in one place

skeeto commented 6 years ago

How's 143d0ca work for you? Shouldn't have to patch it now.

aleks-f commented 6 years ago

there's more to do because without stdbool.h, there's no bool, true, false

how about this:

#if !defined(__cplusplus)
    #if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L)
        #include <stdbool.h>
    #else
    #ifndef bool
        #define bool int
        #define true 1
        #define false 0
    #endif
    #endif // __STDC_VERSION__
#endif // __cplusplus
aleks-f commented 6 years ago

and while you're at it, if it's not too much to ask, here the one last bit I'm also patching:

#if defined(_MSC_VER) && (_MSC_VER < 1900)

#define json_error(json, format, ...)                             \
    if (!(json->flags & JSON_FLAG_ERROR)) {                       \
        json->flags |= JSON_FLAG_ERROR;                           \
        _snprintf_s(json->errmsg, sizeof(json->errmsg), _TRUNCATE,\
                 "error: %lu: " format,                           \
                 (unsigned long) json->lineno,                    \
                 __VA_ARGS__);                                    \
    }                                                             \

#else

#define json_error(json, format, ...)                             \
    if (!(json->flags & JSON_FLAG_ERROR)) {                       \
        json->flags |= JSON_FLAG_ERROR;                           \
        snprintf(json->errmsg, sizeof(json->errmsg),              \
                 "error: %lu: " format,                           \
                 (unsigned long) json->lineno,                    \
                 __VA_ARGS__);                                    \
    }                                                             \

#endif // _MSC_VER
skeeto commented 6 years ago

Aren't bool, true, and false all built directly into C++? Are you also compiling this as C89 + declare anywhere + variadic macros?

aleks-f commented 6 years ago

well, it's kindof complicated. on windows, 1.8.x poco versions still support all the way back to VS 2008, so we resort to compiling as C++ to pacify MSVC. on linux, mac, etc .c files default to C compiler and it's kindof complicated to change the logic for one file, and g++ <=4.8 fails. this is all about to become legacy anyway, we're getting ready to release 2.0 and there minimal compiler versions are VS 2015 and g++ 4.9, so C99 works and there are no any issues with code as-is

bottom line, i'm cool with the initial 3 points I asked for being fixed :)

skeeto commented 6 years ago

Alright, go ahead with a PR with these changes. (Since I'm losing track of what goes where.) My main concern is that it doesn't get in the way of a straight C99 build.

aleks-f commented 6 years ago

good, thanks. I will make sure it compiles with c99 :)