Homogenize printf formatting #825
Conversation
saghul
force-pushed
the
unify-format-printf
branch
from
69d102c to
6e09490
Compare
Hum, maybe not quite :-/ |
saghul
force-pushed
the
unify-format-printf
branch
from
737ac4d to
2470553
Compare
| #define JS_EXTERN /* nothing */ | ||
| #endif | ||
|
|
||
| /* Borrowed from Folly */ |
There was a problem hiding this comment.
Not a huge fan of having this duplicated, but I'm not sure how to avoid it...
saghul
force-pushed
the
unify-format-printf
branch
3 times, most recently
from
8063bc9 to
c18ca3b
Compare
saghul
force-pushed
the
unify-format-printf
branch
from
c18ca3b to
e450007
Compare
|
@bnoordhuis This is now ready, PTAL when you get a chance. |
| /* Borrowed from Folly */ | ||
| #ifndef JS_PRINTF_FORMAT | ||
| #ifdef _MSC_VER | ||
| #include <sal.h> | ||
| #define JS_PRINTF_FORMAT _Printf_format_string_ | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) | ||
| #else | ||
| #define JS_PRINTF_FORMAT | ||
| #if !defined(__clang__) && defined(__GNUC__) | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) \ | ||
| __attribute__((format(gnu_printf, format_param, dots_param))) | ||
| #else | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) \ | ||
| __attribute__((format(printf, format_param, dots_param))) | ||
| #endif | ||
| #endif | ||
| #endif | ||
|
|
There was a problem hiding this comment.
What about compilers that define neither __clang__ nor __GNUC__ ? Do you rely on the fact that __attribute__() expands to nothing?
There was a problem hiding this comment.
This code assumes __attribute__((format(printf aka the "standard". will work. If when we find a compiler where that's not true, we can add an extra check.
| #ifndef JS_PRINTF_FORMAT | ||
| #ifdef _MSC_VER | ||
| #include <sal.h> | ||
| #define JS_PRINTF_FORMAT _Printf_format_string_ | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) | ||
| #else | ||
| #define JS_PRINTF_FORMAT | ||
| #if !defined(__clang__) && defined(__GNUC__) | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) \ | ||
| __attribute__((format(gnu_printf, format_param, dots_param))) | ||
| #else | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) \ | ||
| __attribute__((format(printf, format_param, dots_param))) | ||
| #endif | ||
| #endif | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Duplicating these definitions should definitely be avoided, but this would require either 2 separate public header files or including quickjs.h in places where we aim to avoid it. Neither approach is satisfying.
There was a problem hiding this comment.
Yeah, I agree. Since it's a small piece I went with duplication. I'm open to other ideas!
There was a problem hiding this comment.
Quick testing suggests including quickjs.h in cutils.h Just Works™ but I don't have a strong opinion on what approach is better. If you think duplication is best, go for it.
| #ifndef JS_PRINTF_FORMAT | ||
| #ifdef _MSC_VER | ||
| #include <sal.h> | ||
| #define JS_PRINTF_FORMAT _Printf_format_string_ | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) | ||
| #else | ||
| #define JS_PRINTF_FORMAT | ||
| #if !defined(__clang__) && defined(__GNUC__) | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) \ | ||
| __attribute__((format(gnu_printf, format_param, dots_param))) | ||
| #else | ||
| #define JS_PRINTF_FORMAT_ATTR(format_param, dots_param) \ | ||
| __attribute__((format(printf, format_param, dots_param))) | ||
| #endif | ||
| #endif | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Quick testing suggests including quickjs.h in cutils.h Just Works™ but I don't have a strong opinion on what approach is better. If you think duplication is best, go for it.
|
We use cutils.h in a number of places, just in case, I'll go with duplication for now. |
The
__attribute__(format(printf(a, b)))syntax is supported in MSVC since version 2015 (we require 2019).Add some helper macros while we're at it.