Re: [PATCH 1/2] msvcp110: Add _File_size implementation and test.

[Piotr Caban]

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

[PATCH 1/2] msvcp110: Add _File_size implementation and test.

[Piotr Caban]

> - 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.

[PATCH 1/2] msvcp110: Add _File_size implementation and test.

[YongHao Hu]

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