On Saturday, 8 Nov 2014 11:26:44 +0100, Jonathan Vollebregt wrote:
+static void reg_print_error(LSTATUS error_code) +{ + switch (error_code) + { + case ERROR_SUCCESS: + return; + case ERROR_BAD_COMMAND: + reg_message(STRING_INVALID_CMDLINE); + return; + default: + { + static const WCHAR error_string[] = {'%','0','5','d',':',' ','%','s',0}; + WCHAR *message = NULL; + FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER, NULL, + error_code, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (WCHAR *)&message, 0, NULL); + + reg_message(STRING_ERROR); + reg_printfW(error_string, error_code, message); + LocalFree(message); + return; + } + } +}This seems like a step backwards to me. It would be much better to add the required functionality -- e.g. FormatMessage() -- and fix reg.exe printf-style functions in one go.
See http://source.winehq.org/patches/data/107578 for what I mean.
Also, mixing reg_message() and reg_printfW() just seems wrong. A clear resource string incorporating your formatting above would suffice, I expect.
-- Hugh McMaster
This seems like a step backwards to me. It would be much better to add the required functionality -- e.g. FormatMessage() -- and fix reg.exe printf-style functions in one go.
I'd still have to use reg_print_error as is because it's there to get the error string from standard error codes like ERROR_INVALID_DATA not to actually format the output string (FORMAT_MESSAGE_FROM_SYSTEM vs FORMAT_MESSAGE_FROM_STRING)
I like splitting the writing part into output_write (That might come in handy for printing long strings in reg query if we can't find a way to make reg_printfW variable size)
Giving reg_message extra arguments is a good idea, but I'm not sure if we're supposed to have format specifiers in LoadString resources.
I'm not sure whether having output_vprintf is really an advantage over just calling reg_printf since it uses different format specifiers than printf and basically does the same thing (Minus the variable size issue of course)
On Monday, 10 Nov 2014 14:46:49 +0100, Jonathan Vollebregt wrote:
I'd still have to use reg_print_error as is because it's there to get the error string from standard error codes like ERROR_INVALID_DATA not to actually format the output string (FORMAT_MESSAGE_FROM_SYSTEM vs FORMAT_MESSAGE_FROM_STRING)
Giving reg_message extra arguments is a good idea, but I'm not sure if we're supposed to have format specifiers in LoadString resources.
I'm not sure whether having output_vprintf is really an advantage over just calling reg_printf since it uses different format specifiers than printf and basically does the same thing (Minus the variable size issue of course)
Format specifiers are perfectly valid in resource strings. See programs/regsvr32/regsvr32.rc as one example.
Not having to deal with variable memory allocation is the obvious advantage of using output_vprintf(). My personal choice would be to convert static const WCHAR nonnumber[] and static const WCHAR unhandled[] to resource strings and using reg_message(). These messages really should be translated anyway.
That leaves the three static const WCHAR stubW[] arrays to consider. Personally, I would remove them. They serve no real purpose and native reg doesn't have anything like them.
As to the getting the error message from the system table, I came up with two possibilities.
static void __cdecl reg_printfW(const WCHAR* msg, ...) { __ms_va_list va_args;
__ms_va_start(va_args, msg); output_vprintf(msg, va_args); __ms_va_end(va_args); }
static void reg_print_error(LSTATUS error_code) { WCHAR *str, *buffer; int len, extra_char = 8; static const WCHAR error_string1[] = {'%','1','!','*','.','*','d','!',':',' ','%','3',0}; static const WCHAR error_string2[] = {'%','0','5','d',':',' ','%','s',0};
len = FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM|FORMAT_MESSAGE_ALLOCATE_BUFFER, NULL, error_code, 0, (LPWSTR)&str, 0, NULL); if (len == 0 && GetLastError() != NO_ERROR) { WINE_FIXME("Could not format system string: le=%u\n", GetLastError()); return; } reg_printfW(error_string1, 0, 5, error_code, str); /* Option 1 - fully automated */ /* Option 2 */ buffer = HeapAlloc(GetProcessHeap(), 0, (len + extra_char) * sizeof(WCHAR)); if (buffer) { sprintfW(buffer, error_string2, error_code, str); output_write(buffer, strlenW(buffer)); HeapFree(GetProcessHeap(), 0, buffer); } LocalFree(str); }
The reg patches have been languishing on the list for a while now.
As Hugh's example showed format specifiers are the preferred way of doing rc strings, it just seems a shame we have to duplicate so much.
Can we have 107580 committed so I can rebase on it?
I still wish there were a better way to deal with the fixed-size reg_printfW output but it looks like there is none.
-
Hugh McMaster -
Jonathan Vollebregt