Re: [2/5] dinput: SetActionMap saving simple configurations to an .ini file (try 2)
On 01/09/2012 10:18 AM, Lucas Fialho Zawacki wrote:
From: Lucas Fialho Zawacki<lfzawacki(a)gmail.com>
+static BOOL _write_private_profile_intW(const char *format, WCHAR* section, WCHAR* key, int value, WCHAR* file) I don't think this is such a good idea to mix ASCII and WCHAR parameters.
+ static WCHAR path[] = { + '%','C','o','m','m','o','n','P','r','o','g','r','a','m','F','i','l','e','s','%','\\', + 'D','i','r','e','c','t','X','\\', + 'D','i','r','e','c','t','I','n','p','u','t','\\', + 'U','s','e','r',' ','M','a','p','s','\0'}; Why do you think it should be there in the first place?
+ expanded_path = HeapAlloc(GetProcessHeap(), 0, sizeof(WCHAR)*expanded_path_size); No error checking.
+static HRESULT _save_mapping_settings(IDirectInputDevice8W *iface, LPDIACTIONFORMATW lpdiaf, LPCWSTR lpszUsername) +{ + static WCHAR mapexists[] = {'M','a','p','E','x','i','s','t','s','\0'}; + static WCHAR numactions[] = {'N','u','m','A','c','t','i','o','n','\0'}; Should be "static const WCHAR".
+ IDirectInputDevice8_GetDeviceInfo(iface,&didev); No error checking.
+ va_start(args, format); + while (1) + { + buffer = HeapAlloc(GetProcessHeap(), 0, size); + if (buffer == NULL) + break; + n = vsnprintfW(buffer, size, formatW, args); + if (n == -1) + size *= 2; + else if (n>= size) + size = n + 1; + else + break; + HeapFree(GetProcessHeap(), 0, buffer); + } + va_end(args); You can't do this. It seems to be you have not tested it with initial buffer too small. You have to do va_start & va_end every time you pass "args" to another function.
+ + if (buffer == NULL) return NULL; + ret = HeapReAlloc(GetProcessHeap(), 0, buffer, sizeof(WCHAR)*(strlenW(buffer)+1) ); + if (ret == NULL) ret = buffer; Why reallocate? It's pointless.
+extern WCHAR* _get_mapping_path(const WCHAR *device, const WCHAR *username) DECLSPEC_HIDDEN; Would you please drop leading underscore from all of your function names?
Vitaliy.
2012/1/10 Vitaliy Margolen <wine-devel(a)kievinfo.com>:
On 01/09/2012 10:18 AM, Lucas Fialho Zawacki wrote:
From: Lucas Fialho Zawacki<lfzawacki(a)gmail.com>
+static BOOL _write_private_profile_intW(const char *format, WCHAR* section, WCHAR* key, int value, WCHAR* file)
I don't think this is such a good idea to mix ASCII and WCHAR parameters. Even if this is for convenience in my "private" code? I use this a great deal through the code.
+ static WCHAR path[] = { + '%','C','o','m','m','o','n','P','r','o','g','r','a','m','F','i','l','e','s','%','\\', + 'D','i','r','e','c','t','X','\\', + 'D','i','r','e','c','t','I','n','p','u','t','\\', + 'U','s','e','r',' ','M','a','p','s','\0'};
Why do you think it should be there in the first place? Windows keeps it there, you can check it by running any application using SetActionMap. But I suppose no application depends on it being there specifically.
You can't do this. It seems to be you have not tested it with initial buffer too small. You have to do va_start & va_end every time you pass "args" to another function.
Yes, you're right. There were other mistakes as well. Is this version better? ... while (1) { /* Test if it's the first time */ if (buffer == NULL) buffer = HeapAlloc(GetProcessHeap(), 0, size*sizeof(WCHAR)); else buffer = HeapReAlloc(GetProcessHeap(), 0, buffer, size*sizeof(WCHAR)); if (buffer == NULL) break; va_start(args, format); n = vsnprintfW(buffer, size, formatW, args); va_end(args); if (n == -1) size *= 2; else if (n >= size) size = n + 1; else break; } HeapFree(GetProcessHeap(), 0, formatW); return buffer; I tested it for smaller buffer values. If that's alright I can send a patch to winemenubuilder.c too because the heap_printf there should suffer from the same bug.
Would you please drop leading underscore from all of your function names? Ok. I can stop doing this from this patch onwards if you think this convention is bad.
On 01/10/2012 10:51 AM, Lucas Zawacki wrote:
2012/1/10 Vitaliy Margolen<wine-devel(a)kievinfo.com>:
On 01/09/2012 10:18 AM, Lucas Fialho Zawacki wrote:
From: Lucas Fialho Zawacki<lfzawacki(a)gmail.com>
+static BOOL _write_private_profile_intW(const char *format, WCHAR* section, WCHAR* key, int value, WCHAR* file)
I don't think this is such a good idea to mix ASCII and WCHAR parameters. Even if this is for convenience in my "private" code? I use this a great deal through the code. Always converting static text from ASCII to Unicode during run-time is not a good thing. Yes it sucks that we have to define all Unicode strings as arrays. But such as life.
Why do you think it should be there in the first place? Windows keeps it there, you can check it by running any application using SetActionMap. But I suppose no application depends on it being there specifically. Just because Windows does something is not good enough of an excuse to do the same. AJ said that on many occasions... Here it seems really suspicious that any user can create files inside a program install directory structure. And those files not even executables or libraries... If anything these files belongs under user's profile local data files directory.
Yes, you're right. There were other mistakes as well. Is this version better?
... while (1) { /* Test if it's the first time */ if (buffer == NULL) buffer = HeapAlloc(GetProcessHeap(), 0, size*sizeof(WCHAR)); else buffer = HeapReAlloc(GetProcessHeap(), 0, buffer, size*sizeof(WCHAR)); Nope, you clobber original value of "buffer" and what you had before is lost. So in case it fails you leaking old buffer.
However I really don't like this function, and how it's used. If all you need is to format some strings, then you should provide your own buffer for such a function and let it grow it if needed to. Then you save time by not allocating / freeing same buffer multiple times.
Would you please drop leading underscore from all of your function names? Ok. I can stop doing this from this patch onwards if you think this convention is bad. In *NIX world only system functions deserve leading underscore...
- Vitaliy
participants (2)
-
Lucas Zawacki -
Vitaliy Margolen