Hi,
On 11/02/15 11:08, Martin Storsjo wrote:
@@ -724,12 +724,13 @@ int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char *str, MS struct _str_ctx_a ctx = {len, str}; int ret;
- if (options != UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION &&
options != UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR)
- if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK) FIXME("options %s not handled\n", wine_dbgstr_longlong(options));
- ret = pf_printf_a(options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? puts_clbk_str_c99_a : puts_clbk_str_a,
- ret = pf_printf_a(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION ? puts_clbk_str_a : puts_clbk_str_c99_a, &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, &valist); puts_clbk_str_a(&ctx, 1, &nullbyte);
- if ((options & UCRTBASE_PRINTF_TERMINATION_MASK) == 0 && ret >= len)
}ret = -2; return ret;
This code looks like you're trying to make the things other way around. Shouldn't UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR flag impact the callback being used? On the other hand UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION flag should probably only affect terminating NULL related behavior.
Maybe following code makes more sense: ret = pf_printf_a(options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? puts_clbk_str_c99_a : puts_clbk_str_a, &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, &valist); if(puts_clbk_str_a(&ctx, 1, &nullbyte)==-1 && !(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION)) { if(len) str[len-1] = 0; return -2; } return ret;
What do you think about it?
Thanks, Piotr
On Mon, 2 Nov 2015, Piotr Caban wrote:
Hi,
On 11/02/15 11:08, Martin Storsjo wrote:
@@ -724,12 +724,13 @@ int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char *str, MS struct _str_ctx_a ctx = {len, str}; int ret;
- if (options != UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION &&
options != UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR)
- if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK) FIXME("options %s not handled\n", wine_dbgstr_longlong(options));
- ret = pf_printf_a(options &
UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? puts_clbk_str_c99_a : puts_clbk_str_a,
- ret = pf_printf_a(options &
UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION ? puts_clbk_str_a : puts_clbk_str_c99_a, &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, &valist); puts_clbk_str_a(&ctx, 1, &nullbyte);
- if ((options & UCRTBASE_PRINTF_TERMINATION_MASK) == 0 && ret >= len)
}ret = -2; return ret;
This code looks like you're trying to make the things other way around. Shouldn't UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR flag impact the callback being used? On the other hand UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION flag should probably only affect terminating NULL related behavior.
Actually, I don't think of them as two separate flags, but as having 3 different modes (with the fourth, with both flags being set, undefined). But I see what you're getting at.
Maybe following code makes more sense: ret = pf_printf_a(options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? puts_clbk_str_c99_a : puts_clbk_str_a, &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, &valist); if(puts_clbk_str_a(&ctx, 1, &nullbyte)==-1 && !(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION)) { if(len) str[len-1] = 0; return -2; } return ret;
What do you think about it?
I guess this would work as well, although it took me a little while to figure out why (why the return -2 case won't be invoked when the standard snprintf behaviour is requested - because the nullbyte write will always succeed since the c99 callback left space for it).
After thinking another few minutes about it - sure, this is fine for me. Should I resend the full patchset, or only 4->10 (at least some of them will need some conflict resolution)? If I resend the full patchset, I guess I can fold in your sign-offs in 1-3 (as long as I don't modify those patches)?
// Martin
Hi,
I don't like the code I've proposed in last email. Something like this seems to be nicer: static int puts_clbk_str_c99_a(void *ctx, int len, const char *str) { struct _str_ctx_a *out = ctx;
if(!out->buf) return len;
if(out->len < len) { memcpy(out->buf, str, out->len); out->buf += out->len; out->len = 0; return len; }
memcpy(out->buf, str, len); out->buf += len; out->len -= len; return len; }
/********************************************************************* * __stdio_common_vsprintf (MSVCRT.@) */ int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char *str, MSVCRT_size_t len, const char *format, MSVCRT__locale_t locale, __ms_va_list valist ) { static const char nullbyte = '\0'; struct _str_ctx_a ctx = {len, str}; int ret;
if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK) FIXME("options %s not handled\n", wine_dbgstr_longlong(options)); ret = pf_printf_a(puts_clbk_str_c99_a, &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, &valist); puts_clbk_str_a(&ctx, 1, &nullbyte);
if(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION) return ret>len ? -1 : ret; if(ret>=len) { if(len) str[len-1] = 0; return (options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR) ? ret : -2; } return ret; } if you think that your initial code is better please let me know. Probably in this code UCRTBASE_PRINTF_TERMINATION_MASK define is not needed.
On 11/02/15 13:45, Martin Storsjö wrote:
After thinking another few minutes about it - sure, this is fine for me. Should I resend the full patchset, or only 4->10 (at least some of them will need some conflict resolution)? If I resend the full patchset, I guess I can fold in your sign-offs in 1-3 (as long as I don't modify those patches)?
I can sign the patches after you resend them myself (I don't know what's the policy in this case).
Thanks, Piotr
Piotr Caban piotr.caban@gmail.com writes:
On 11/02/15 13:45, Martin Storsjö wrote:
After thinking another few minutes about it - sure, this is fine for me. Should I resend the full patchset, or only 4->10 (at least some of them will need some conflict resolution)? If I resend the full patchset, I guess I can fold in your sign-offs in 1-3 (as long as I don't modify those patches)?
I can sign the patches after you resend them myself (I don't know what's the policy in this case).
I prefer if you sign them again after a resend, if it's folded into the patch email I won't notice it right away. But if the already signed off patches are still valid, it's even better to wait for me to commit them, and then resend only the remaining ones.
On Mon, 2 Nov 2015, Alexandre Julliard wrote:
Piotr Caban piotr.caban@gmail.com writes:
On 11/02/15 13:45, Martin Storsjö wrote:
After thinking another few minutes about it - sure, this is fine for me. Should I resend the full patchset, or only 4->10 (at least some of them will need some conflict resolution)? If I resend the full patchset, I guess I can fold in your sign-offs in 1-3 (as long as I don't modify those patches)?
I can sign the patches after you resend them myself (I don't know what's the policy in this case).
I prefer if you sign them again after a resend, if it's folded into the patch email I won't notice it right away. But if the already signed off patches are still valid, it's even better to wait for me to commit them, and then resend only the remaining ones.
They are still valid, so I'll resend the rest of the branch later after today's push.
// Martin
On Mon, 2 Nov 2015, Piotr Caban wrote:
Hi,
I don't like the code I've proposed in last email. Something like this seems to be nicer: static int puts_clbk_str_c99_a(void *ctx, int len, const char *str) { struct _str_ctx_a *out = ctx;
if(!out->buf) return len;
if(out->len < len) { memcpy(out->buf, str, out->len); out->buf += out->len; out->len = 0; return len; }
memcpy(out->buf, str, len); out->buf += len; out->len -= len; return len; }
/*********************************************************************
__stdio_common_vsprintf (MSVCRT.@)
*/ int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char *str, MSVCRT_size_t len, const char *format, MSVCRT__locale_t locale, __ms_va_list valist ) { static const char nullbyte = '\0'; struct _str_ctx_a ctx = {len, str}; int ret;
if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK) FIXME("options %s not handled\n", wine_dbgstr_longlong(options)); ret = pf_printf_a(puts_clbk_str_c99_a, &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, &valist); puts_clbk_str_a(&ctx, 1, &nullbyte);
if(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION) return ret>len ? -1 : ret; if(ret>=len) { if(len) str[len-1] = 0; return (options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR) ? ret : -2; } return ret; } if you think that your initial code is better please let me know.
No, this feels like the most straightforward and explicit way of implementing it so far - thanks!
Probably in this code UCRTBASE_PRINTF_TERMINATION_MASK define is not needed.
I'd keep it here (as condition for the fixme warning - alternatively add "|| options == 0" to the condition), but remove it once that warning is removed in a later patch.
// Martin