Re: [PATCH 3/3] kernel32/tests: Add a test for opening short paths of differing case.
On 10/27/2010 07:46 PM, Charles Davis wrote:
+ sprintf(buf,"%s\\%s\\%s",tmpdir,dirname,filename); Could you please put some spaces after each coma everywhere in your patch? Please use snprintf().
+ GetShortPathNameA(buf,shortbuf,MAX_PATH); Use sizeof(buf) instead of MAX_PATH to guarantee that's what it means.
+ hndl = CreateFileA(shortbuf,GENERIC_READ|GENERIC_WRITE,0,NULL,OPEN_EXISTING,0,NULL); + ok(hndl!=INVALID_HANDLE_VALUE,"CreateFileA failed\n"); It's nice to know LastError and the full filename if it fails.
+ for(i=0;i<MAX_PATH&& shortbuf[i];i++) You getting past the filename size in the buffer.
+ shortbuf[i]=(rand() % 2) ? tolower(shortbuf[i]) : toupper(shortbuf[i]); Please don't use any sort of random stuff in tests. They should always be the same every time on every platform.
+ /* Now try it on mixed case short names */ What's the point of the test anyway? You know that Wine already handles all files as case-insensitive. Short file name isn't any different.
Vitaliy.
On 10/27/10 9:21 PM, Vitaliy Margolen wrote:
On 10/27/2010 07:46 PM, Charles Davis wrote:
+ sprintf(buf,"%s\\%s\\%s",tmpdir,dirname,filename); Could you please put some spaces after each coma everywhere in your patch? Please use snprintf(). I was just following the surrounding style. But yeah, I guess I should use snprintf() on a fixed-size buffer.
+ GetShortPathNameA(buf,shortbuf,MAX_PATH); Use sizeof(buf) instead of MAX_PATH to guarantee that's what it means. Good point.
+ hndl = CreateFileA(shortbuf,GENERIC_READ|GENERIC_WRITE,0,NULL,OPEN_EXISTING,0,NULL);
+ ok(hndl!=INVALID_HANDLE_VALUE,"CreateFileA failed\n"); It's nice to know LastError and the full filename if it fails. The other tests don't do this. That'll teach me to go against my better judgment.
+ for(i=0;i<MAX_PATH&& shortbuf[i];i++) You getting past the filename size in the buffer.
+ shortbuf[i]=(rand() % 2) ? tolower(shortbuf[i]) : toupper(shortbuf[i]); Please don't use any sort of random stuff in tests. They should always be the same every time on every platform. I forgot about that rule. I'll change it to alternate upper and lowercase.
+ /* Now try it on mixed case short names */ What's the point of the test anyway? You know that Wine already handles all files as case-insensitive. Short file name isn't any different. Don't blame me; this was AJ's idea. He told me, "you should probably first write more tests with various case and short names combinations," when he was critiquing patch 2 in this series. I'm just "following orders."
Chip
On 10/27/2010 09:47 PM, Charles Davis wrote:
On 10/27/10 9:21 PM, Vitaliy Margolen wrote:
On 10/27/2010 07:46 PM, Charles Davis wrote:
+ /* Now try it on mixed case short names */ What's the point of the test anyway? You know that Wine already handles all files as case-insensitive. Short file name isn't any different. Don't blame me; this was AJ's idea. He told me, "you should probably first write more tests with various case and short names combinations," when he was critiquing patch 2 in this series. I'm just "following orders."
Sorry about that, read AJ's message after writing this e-mail. Boss is always right :) Vitaliy.
participants (2)
-
Charles Davis -
Vitaliy Margolen