Skip to content

Homogenize printf formatting #825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Homogenize printf formatting #825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions cutils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,7 @@ int dbuf_putstr(DynBuf *s, const char *str)
return dbuf_put(s, (const uint8_t *)str, strlen(str));
}

int __attribute__((format(printf, 2, 3))) dbuf_printf(DynBuf *s,
const char *fmt, ...)
int JS_PRINTF_FORMAT_ATTR(2, 3) dbuf_printf(DynBuf *s, JS_PRINTF_FORMAT const char *fmt, ...)
{
va_list ap;
char buf[128];
Expand Down
34 changes: 19 additions & 15 deletions cutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,6 @@ extern "C" {
# define __maybe_unused __attribute__((unused))
#endif

// https://stackoverflow.com/a/6849629
#undef FORMAT_STRING
#if _MSC_VER >= 1400
# include <sal.h>
# if _MSC_VER > 1400
# define FORMAT_STRING(p) _Printf_format_string_ p
# else
# define FORMAT_STRING(p) __format_string p
# endif /* FORMAT_STRING */
#else
# define FORMAT_STRING(p) p
#endif /* _MSC_VER */

#if defined(_MSC_VER) && !defined(__clang__)
#include <math.h>
#define INF INFINITY
Expand Down Expand Up @@ -113,6 +100,24 @@ extern "C" {
#define minimum_length(n) static n
#endif

/* 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

Comment on lines +103 to +120
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about compilers that define neither __clang__ nor __GNUC__ ? Do you rely on the fact that __attribute__() expands to nothing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

void js__pstrcpy(char *buf, int buf_size, const char *str);
char *js__pstrcat(char *buf, int buf_size, const char *s);
int js__strstart(const char *str, const char *val, const char **ptr);
Expand Down Expand Up @@ -445,8 +450,7 @@ static inline int dbuf_put_u64(DynBuf *s, uint64_t val)
{
return dbuf_put(s, (uint8_t *)&val, 8);
}
int __attribute__((format(printf, 2, 3))) dbuf_printf(DynBuf *s,
FORMAT_STRING(const char *fmt), ...);
int JS_PRINTF_FORMAT_ATTR(2, 3) dbuf_printf(DynBuf *s, JS_PRINTF_FORMAT const char *fmt, ...);
void dbuf_free(DynBuf *s);
static inline bool dbuf_error(DynBuf *s) {
return s->error;
Expand Down
2 changes: 1 addition & 1 deletion libregexp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ static void re_emit_op_u16(REParseState *s, int op, uint32_t val)
dbuf_put_u16(&s->byte_code, val);
}

static int __attribute__((format(printf, 2, 3))) re_parse_error(REParseState *s, const char *fmt, ...)
static int JS_PRINTF_FORMAT_ATTR(2, 3) re_parse_error(REParseState *s, const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
Expand Down
9 changes: 1 addition & 8 deletions qjs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,7 @@ static inline unsigned long long js_trace_malloc_ptr_offset(uint8_t *ptr,
return ptr - dp->base;
}

static void
#if defined(_WIN32) && !defined(__clang__)
/* mingw printf is used */
__attribute__((format(gnu_printf, 2, 3)))
#else
__attribute__((format(printf, 2, 3)))
#endif
js_trace_malloc_printf(void *opaque, const char *fmt, ...)
static void JS_PRINTF_FORMAT_ATTR(2, 3) js_trace_malloc_printf(void *opaque, JS_PRINTF_FORMAT const char *fmt, ...)
{
va_list ap;
int c;
Expand Down
26 changes: 13 additions & 13 deletions quickjs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1093,7 +1093,7 @@ static __exception int JS_ToArrayLengthFree(JSContext *ctx, uint32_t *plen,
JSValue val, bool is_array_ctor);
static JSValue JS_EvalObject(JSContext *ctx, JSValue this_obj,
JSValue val, int flags, int scope_idx);
JSValue __attribute__((format(printf, 2, 3))) JS_ThrowInternalError(JSContext *ctx, const char *fmt, ...);
JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowInternalError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);

static __maybe_unused void JS_DumpString(JSRuntime *rt, const JSString *p);
static __maybe_unused void JS_DumpObjectHeader(JSRuntime *rt);
Expand Down Expand Up @@ -6882,7 +6882,7 @@ static JSValue JS_ThrowError(JSContext *ctx, JSErrorEnum error_num,
return JS_ThrowError2(ctx, error_num, fmt, ap, add_backtrace);
}

JSValue __attribute__((format(printf, 2, 3))) JS_ThrowPlainError(JSContext *ctx, const char *fmt, ...)
JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowPlainError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...)
{
JSValue val;
va_list ap;
Expand All @@ -6893,7 +6893,7 @@ JSValue __attribute__((format(printf, 2, 3))) JS_ThrowPlainError(JSContext *ctx,
return val;
}

JSValue __attribute__((format(printf, 2, 3))) JS_ThrowSyntaxError(JSContext *ctx, const char *fmt, ...)
JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowSyntaxError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...)
{
JSValue val;
va_list ap;
Expand All @@ -6904,7 +6904,7 @@ JSValue __attribute__((format(printf, 2, 3))) JS_ThrowSyntaxError(JSContext *ctx
return val;
}

JSValue __attribute__((format(printf, 2, 3))) JS_ThrowTypeError(JSContext *ctx, const char *fmt, ...)
JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowTypeError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...)
{
JSValue val;
va_list ap;
Expand All @@ -6915,7 +6915,7 @@ JSValue __attribute__((format(printf, 2, 3))) JS_ThrowTypeError(JSContext *ctx,
return val;
}

static int __attribute__((format(printf, 3, 4))) JS_ThrowTypeErrorOrFalse(JSContext *ctx, int flags, const char *fmt, ...)
static int JS_PRINTF_FORMAT_ATTR(3, 4) JS_ThrowTypeErrorOrFalse(JSContext *ctx, int flags, JS_PRINTF_FORMAT const char *fmt, ...)
{
va_list ap;

Expand All @@ -6931,15 +6931,15 @@ static int __attribute__((format(printf, 3, 4))) JS_ThrowTypeErrorOrFalse(JSCont
}

/* never use it directly */
static JSValue __attribute__((format(printf, 3, 4))) __JS_ThrowTypeErrorAtom(JSContext *ctx, JSAtom atom, const char *fmt, ...)
static JSValue JS_PRINTF_FORMAT_ATTR(3, 4) __JS_ThrowTypeErrorAtom(JSContext *ctx, JSAtom atom, JS_PRINTF_FORMAT const char *fmt, ...)
{
char buf[ATOM_GET_STR_BUF_SIZE];
return JS_ThrowTypeError(ctx, fmt,
JS_AtomGetStr(ctx, buf, sizeof(buf), atom));
}

/* never use it directly */
static JSValue __attribute__((format(printf, 3, 4))) __JS_ThrowSyntaxErrorAtom(JSContext *ctx, JSAtom atom, const char *fmt, ...)
static JSValue JS_PRINTF_FORMAT_ATTR(3, 4) __JS_ThrowSyntaxErrorAtom(JSContext *ctx, JSAtom atom, JS_PRINTF_FORMAT const char *fmt, ...)
{
char buf[ATOM_GET_STR_BUF_SIZE];
return JS_ThrowSyntaxError(ctx, fmt,
Expand All @@ -6962,7 +6962,7 @@ static int JS_ThrowTypeErrorReadOnly(JSContext *ctx, int flags, JSAtom atom)
}
}

JSValue __attribute__((format(printf, 2, 3))) JS_ThrowReferenceError(JSContext *ctx, const char *fmt, ...)
JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowReferenceError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...)
{
JSValue val;
va_list ap;
Expand All @@ -6973,7 +6973,7 @@ JSValue __attribute__((format(printf, 2, 3))) JS_ThrowReferenceError(JSContext *
return val;
}

JSValue __attribute__((format(printf, 2, 3))) JS_ThrowRangeError(JSContext *ctx, const char *fmt, ...)
JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowRangeError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...)
{
JSValue val;
va_list ap;
Expand All @@ -6984,7 +6984,7 @@ JSValue __attribute__((format(printf, 2, 3))) JS_ThrowRangeError(JSContext *ctx,
return val;
}

JSValue __attribute__((format(printf, 2, 3))) JS_ThrowInternalError(JSContext *ctx, const char *fmt, ...)
JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowInternalError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...)
{
JSValue val;
va_list ap;
Expand Down Expand Up @@ -19034,7 +19034,7 @@ static void __attribute((unused)) dump_token(JSParseState *s,
}
}

int __attribute__((format(printf, 2, 3))) js_parse_error(JSParseState *s, const char *fmt, ...)
int JS_PRINTF_FORMAT_ATTR(2, 3) js_parse_error(JSParseState *s, JS_PRINTF_FORMAT const char *fmt, ...)
{
JSContext *ctx = s->ctx;
va_list ap;
Expand Down Expand Up @@ -34647,7 +34647,7 @@ typedef struct BCReaderState {
} BCReaderState;

#ifdef DUMP_READ_OBJECT
static void __attribute__((format(printf, 2, 3))) bc_read_trace(BCReaderState *s, const char *fmt, ...) {
static void JS_PRINTF_FORMAT_ATTR(2, 3) bc_read_trace(BCReaderState *s, JS_PRINTF_FORMAT const char *fmt, ...) {
va_list ap;
int i, n, n0;

Expand Down Expand Up @@ -49924,7 +49924,7 @@ static int isURIReserved(int c) {
return c < 0x100 && memchr(";/?:@&=+$,#", c, sizeof(";/?:@&=+$,#") - 1) != NULL;
}

static int __attribute__((format(printf, 2, 3))) js_throw_URIError(JSContext *ctx, const char *fmt, ...)
static int JS_PRINTF_FORMAT_ATTR(2, 3) js_throw_URIError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...)
{
va_list ap;

Expand Down
33 changes: 24 additions & 9 deletions quickjs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,30 @@ extern "C" {

#if defined(__GNUC__) || defined(__clang__)
#define js_force_inline inline __attribute__((always_inline))
#define __js_printf_like(f, a) __attribute__((format(printf, f, a)))
#define JS_EXTERN __attribute__((visibility("default")))
#else
#define js_force_inline inline
#define __js_printf_like(a, b)
#define JS_EXTERN /* nothing */
#endif

/* Borrowed from Folly */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge fan of having this duplicated, but I'm not sure how to avoid 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

Comment on lines +49 to +65
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. Since it's a small piece I went with duplication. I'm open to other ideas!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

typedef struct JSRuntime JSRuntime;
typedef struct JSContext JSContext;
typedef struct JSObject JSObject;
Expand Down Expand Up @@ -624,12 +640,12 @@ JS_EXTERN void JS_ClearUncatchableError(JSContext *ctx, JSValue val);
// JS_Throw(ctx, exc);
JS_EXTERN void JS_ResetUncatchableError(JSContext *ctx);
JS_EXTERN JSValue JS_NewError(JSContext *ctx);
JS_EXTERN JSValue __js_printf_like(2, 3) JS_ThrowPlainError(JSContext *ctx, const char *fmt, ...);
JS_EXTERN JSValue __js_printf_like(2, 3) JS_ThrowSyntaxError(JSContext *ctx, const char *fmt, ...);
JS_EXTERN JSValue __js_printf_like(2, 3) JS_ThrowTypeError(JSContext *ctx, const char *fmt, ...);
JS_EXTERN JSValue __js_printf_like(2, 3) JS_ThrowReferenceError(JSContext *ctx, const char *fmt, ...);
JS_EXTERN JSValue __js_printf_like(2, 3) JS_ThrowRangeError(JSContext *ctx, const char *fmt, ...);
JS_EXTERN JSValue __js_printf_like(2, 3) JS_ThrowInternalError(JSContext *ctx, const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowPlainError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowSyntaxError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowTypeError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowReferenceError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowRangeError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowInternalError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_ThrowOutOfMemory(JSContext *ctx);
JS_EXTERN void JS_FreeValue(JSContext *ctx, JSValue v);
JS_EXTERN void JS_FreeValueRT(JSRuntime *rt, JSValue v);
Expand Down Expand Up @@ -1075,7 +1091,6 @@ JS_EXTERN uintptr_t js_std_cmd(int cmd, ...);

#undef JS_EXTERN
#undef js_force_inline
#undef __js_printf_like

#ifdef __cplusplus
} /* extern "C" { */
Expand Down
Loading