Re: [PATCH 1/2] msvcp110: Add _File_size implementation and test.
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
+ ok(val == 519396075520 || broken(val == 0), "file_size is %llu\n", val); /* not enough storage on the testbots */ I've missed one more thing. Please don't use %llu format in printf. You can see how to print the value in msvcp90/tests/ios.c file.
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
participants (2)
-
Piotr Caban -
YongHao Hu