Bug 189496 – [Win][Clang][ImageDiff] Fix compilation error and warning of PlatformImageCairo.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fujii Hironori]

[Win][Clang][ImageDiff] Fix compilation error and warning of PlatformImageCairo.cpp

While trying to build WebKit WinCairo port with the latest Clang (Bug 171618),
the following compilation error and warning is reported.

> [2/1360] Building CXX object Tools\ImageDiff\CMakeFiles\ImageDiffLib.dir\cairo\PlatformImageCairo.cpp.obj
> FAILED: Tools/ImageDiff/CMakeFiles/ImageDiffLib.dir/cairo/PlatformImageCairo.cpp.obj 
> C:\tools\llvm\bin\clang-cl.exe  (...) /FdTools\ImageDiff\CMakeFiles\ImageDiffLib.dir\ -c ..\..\Tools\ImageDiff\cairo\PlatformImageCairo.cpp
> ..\..\Tools\ImageDiff\cairo\PlatformImageCairo.cpp(46,22):  error: non-constant-expression cannot be narrowed from type 'size_t' (aka 'unsigned long long') to 'unsigned long' in initializer list [-Wc++11-narrowing]
>     } context { { }, imageSize, 0 };
>                      ^~~~~~~~~
> ..\..\Tools\ImageDiff\cairo\PlatformImageCairo.cpp(46,22):  note: insert an explicit cast to silence this issue
>     } context { { }, imageSize, 0 };
>                      ^~~~~~~~~
>                      static_cast<unsigned long>( )
> ..\..\Tools\ImageDiff\cairo\PlatformImageCairo.cpp(125,61):  warning: format specifies type 'unsigned __int64' (aka 'unsigned long long') but the argument has type 'unsigned long' [-Wformat]
>     fprintf(stdout, "Content-Length: %" FORMAT_SIZE_T "\n", context.writtenBytes);
>                                      ~~~~~~~~~~~~~~~~       ^~~~~~~~~~~~~~~~~~~~
> 1 warning and 1 error generated.

[Fujii Hironori]

Created attachment 349389 [details]
Patch

[Don Olmstead]

Informal r+ from me. Will look for a formal review.

[Per Arne Vollan]

Comment on attachment 349389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349389&action=review

> Tools/ImageDiff/cairo/PlatformImageCairo.cpp:40
> -        }, &context);
> +        }, nullptr);

Can this parameter be nullptr? I think this is passed to the read function in the first parameter.

[Fujii Hironori]

Comment on attachment 349389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349389&action=review

>> Tools/ImageDiff/cairo/PlatformImageCairo.cpp:40
>> +        }, nullptr);
> 
> Can this parameter be nullptr? I think this is passed to the read function in the first parameter.

I don't understand. I removed the unnecessary first argument.

This is the spec.
https://www.cairographics.org/manual/cairo-PNG-Support.html#cairo-read-func-t

[Fujii Hironori]

Just for the record. MSVC also reports following warnings in this source file:

> 10>c:\webkit\ga\tools\imagediff\cairo\platformimagecairo.cpp(46): warning C4838: conversion from 'size_t' to 'unsigned long' requires a narrowing conversion
> 10>c:\webkit\ga\tools\imagediff\cairo\platformimagecairo.cpp(125): warning C4477: 'fprintf' : format string '%Iu' requires an argument of type 'unsigned __int64', but variadic argument 1 has type 'unsigned long'
> 10>c:\webkit\ga\tools\imagediff\cairo\platformimagecairo.cpp(125): note: consider using '%lu' in the format string

[Fujii Hironori]

Comment on attachment 349389 [details]
Patch

Clearing flags on attachment: 349389

Committed r235929: <https://trac.webkit.org/changeset/235929>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44370981>

[Frédéric Wang (:fredw)]

Comment on attachment 349389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349389&action=review

>>> Tools/ImageDiff/cairo/PlatformImageCairo.cpp:40
>>> +        }, nullptr);
>> 
>> Can this parameter be nullptr? I think this is passed to the read function in the first parameter.
> 
> I don't understand. I removed the unnecessary first argument.
> 
> This is the spec.
> https://www.cairographics.org/manual/cairo-PNG-Support.html#cairo-read-func-t

Note that this is causing compilation warning now that imageSize is unused. I think you can just write "PlatformImage::createFromStdin(size_t)" now.

[Fujii Hironori]

Thank you for letting me know. I will fix.

[Fujii Hironori]

Committed r235965: <https://trac.webkit.org/changeset/235965>