[PATCH 1/2] msvcp110: Add _File_size implementation and test.
YongHao Hu
christopherwuy at gmail.com
Wed May 27 20:50:18 CDT 2015
Hi, Piotr.
Thank you very much for all your comments. They are of great help.
On 2015年05月27日 18:30, Piotr Caban wrote:
> On 05/27/15 04:35, YongHao Hu wrote:
>> +/* filesystem */
>> +static unsigned long long (__cdecl*p_tr2_sys__File_size)(char const*);
> You should avoid using "long long" type. Please e.g. use ULONGLONG
> instead.
>
>> + mkdir("tr2_test_dir");
>> + file = CreateFileA("tr2_test_dir/f1", GENERIC_WRITE, 0, NULL,
>> CREATE_ALWAYS, 0, NULL);
>> + SetFilePointerEx(file, large, NULL, FILE_BEGIN);
>> + SetEndOfFile(file);
> Please test return values of CreateFile, SetFilePointerEx and
> SetEndOfFile.
>
>> + large.LowPart = 4000000000u;
>> + large.HighPart = 120;
>> + file = CreateFileA("tr2_test_dir/f2", GENERIC_WRITE, 0, NULL,
>> CREATE_ALWAYS, 0, NULL);
>> + SetFilePointerEx(file, large, NULL, FILE_BEGIN);
>> + SetEndOfFile(file);
>> + val = p_tr2_sys__File_size("tr2_test_dir/f2");
>> + ok(val == 519396075520 || broken(val == 0), "file_size is
>> %llu\n", val); /* not enough storage on the testbots */
>> + CloseHandle(file);
> Please remove this test. It's not a good idea to create ~500GB file in
> the tests.
>
>> + val = p_tr2_sys__File_size("tr2_test_dir");
>> + ok(val == 0, "file_size is %llu\n", val);
>> +
>> + val = p_tr2_sys__File_size("tr2_test_dir/not_exists_file");
>> + ok(val == 0, "file_size is %llu\n", val);
> It would be nice to test GetLastError and errno value after this calls.
>
> What happens if NULL is passed as the argument?
>
>> +ULONGLONG __cdecl tr2_sys__File_size(const char* path)
>> +{
>> + WIN32_FILE_ATTRIBUTE_DATA fad;
> Please add a trace message.
>
>> + if(!GetFileAttributesExA(path, GetFileExInfoStandard, &fad))
>> + return (ULONGLONG)0;
> This cast is not needed, you can just return 0 here.
>
>> + if(fad.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
>> + return (ULONGLONG)0;
>> + return ((ULONGLONG)(fad.nFileSizeHigh) <<
>> (sizeof(fad.nFileSizeLow)*8)) + fad.nFileSizeLow;
> I think that it's better to simply write 32 instead of
> sizeof(fad.nFileSizeLow)*8 here.
>
> Thanks,
> Piotr
More information about the wine-devel
mailing list