Comment on attachment 131251[details]
Warnings - Signed/Unsigned mismatches (merged)
WebKit style is to use C++ casts, not C casts.
I would encourage you to post patches to separate bugs, so that domain experts could more easily be consulted (for instance, I don't really want to CC JSC experts on this mostly Blackberry specific bug).
(In reply to comment #15)
> (From update of attachment 131251[details])
> WebKit style is to use C++ casts, not C casts.
Only the IconDatabase one is C-style and was following the pattern already used (and reviewed) there. I can follow it up with a patch to change the style after if really desired.
> I would encourage you to post patches to separate bugs, so that domain experts could more easily be consulted (for instance, I don't really want to CC JSC experts on this mostly Blackberry specific bug).
I don't think they're blackberry specific. This is GCC 4.6. I split the blackberry specific ones out.
(In reply to comment #16)
> (From update of attachment 131254[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131254&action=review
>
> > Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:-88
> > - struct jpeg_compress_struct compressData = { 0 };
>
> What's improper about this initialization?
GCC feels that only the first element in the structure is being initialized.
[ 9%] Building CXX object Source/WebCore/CMakeFiles/webcore.dir/platform/image-encoders/JPEGImageEncoder.cpp.o
Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp: In function 'bool WebCore::compressRGBABigEndianToJPEG(unsigned char*, const WebCore::IntSize&, WTF::Vector<char>&)':
Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:89:52: warning: missing initializer for member 'jpeg_compress_struct::mem' [-Wmissing-field-initializers]
Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:89:52: warning: missing initializer for member 'jpeg_compress_struct::progress' [-Wmissing-field-initializers]
Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:89:52: warning: missing initializer for member 'jpeg_compress_struct::client_data' [-Wmissing-field-initializers]
[....]
If true, this would likely point to a real bug.
Comment on attachment 131254[details]
Properly initialize JPEG structure
View in context: https://bugs.webkit.org/attachment.cgi?id=131254&action=review>>> Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:-88
>>> - struct jpeg_compress_struct compressData = { 0 };
>>
>> What's improper about this initialization?
>
> GCC feels that only the first element in the structure is being initialized.
>
>
> [ 9%] Building CXX object Source/WebCore/CMakeFiles/webcore.dir/platform/image-encoders/JPEGImageEncoder.cpp.o
> Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp: In function 'bool WebCore::compressRGBABigEndianToJPEG(unsigned char*, const WebCore::IntSize&, WTF::Vector<char>&)':
> Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:89:52: warning: missing initializer for member 'jpeg_compress_struct::mem' [-Wmissing-field-initializers]
> Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:89:52: warning: missing initializer for member 'jpeg_compress_struct::progress' [-Wmissing-field-initializers]
> Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:89:52: warning: missing initializer for member 'jpeg_compress_struct::client_data' [-Wmissing-field-initializers]
> [....]
>
> If true, this would likely point to a real bug.
I'm pretty sure that it just warns about default initialization to 0.
Will it shut up if you remove the zero? AFAIK it's necessary in C, but not in C++. "struct" is unnecessary, too.
jpeg_compress_struct compressData = {};
In fact, {0} is theoretically more correct than memset - a null pointer is not necessarily all zero bits. We don't aim to support such platforms in WebKit, of course.
> Only the IconDatabase one is C-style and was following the pattern already used (and reviewed) there. I can follow it up with a patch to change the style after if really desired.
I'm not sure what you mean by this. For example, you're adding a C cast in MacroAssemblerARM.h:
- ARMWord tmp = (right.m_value == 0x80000000) ? ARMAssembler::INVALID_IMM : m_assembler.getOp2(-right.m_value);
+ ARMWord tmp = (right.m_value == signed(0x80000000)) ? ARMAssembler::INVALID_IMM : m_assembler.getOp2(-right.m_value);
Even if there are other instances of this in the same file, it doesn't mean that it's an exception to coding style. Also, we don't use "signed", we use "int".
I still think that it would be useful for a JSC expert to have a look, and it's more appropriate to ask for such in a separate bug.
(In reply to comment #22)
> > Only the IconDatabase one is C-style and was following the pattern already used (and reviewed) there. I can follow it up with a patch to change the style after if really desired.
>
> I'm not sure what you mean by this. For example, you're adding a C cast in MacroAssemblerARM.h:
>
> - ARMWord tmp = (right.m_value == 0x80000000) ? ARMAssembler::INVALID_IMM : m_assembler.getOp2(-right.m_value);
> + ARMWord tmp = (right.m_value == signed(0x80000000)) ? ARMAssembler::INVALID_IMM : m_assembler.getOp2(-right.m_value);
>
> Even if there are other instances of this in the same file, it doesn't mean that it's an exception to coding style. Also, we don't use "signed", we use "int".
>
> I still think that it would be useful for a JSC expert to have a look, and it's more appropriate to ask for such in a separate bug.
At least until recently that's not valid C. I'm happy to change it to int from signed. I'll make it static_cast<> even if that's what you want, but it's definitely C++ as it is.
(In reply to comment #25)
> (From update of attachment 131254[details])
> Clearing flags for now, please investigate suggestion in comment 21.
Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:88:49: warning: missing initializer for member 'jpeg_compress_struct::err' [-Wmissing-field-initializers]
Same issue.
Comment on attachment 131254[details]
Properly initialize JPEG structure
re-requesting review. As far as I can tell this is correct and GCC thinks all the alternatives are not correct.
(In reply to comment #31)
> Well, as mentioned above, memset is not rigorously correct, so there must be a better solution.
It's a C library so there is no constructor. initializing each member directly is error-prone. Another option is we just don't initialize it? I just don't see another option.
(In reply to comment #33)
> I'd suggest either initializing each member, or silencing down this warning for gcc.
This is bizarre and truly beyond me. The warning is saying something is wrong. Covering your eyes doesn't make the boogie man go away. Either the compiler is broken (please show me - I wouldn't deny it) or the code is wrong. If the code is wrong, initializing each member is the most error-prone choice of solution.
It is clearly undergoing churn:
http://digikam.1695700.n4.nabble.com/Problems-compiling-digikam-on-Mac-td3480579.html
Examples like this seem to indicate that there is no need to initialize the members:
http://www.lavrsen.dk/svn/motion/trunk/picture.c
However the docs for jpeg_start_compress indicate somewhat otherwise. Initializing seems to be the safest choice.
It presently has this many members at least:
struct jpeg_error_mgr * err
struct jpeg_memory_mgr * mem
struct jpeg_progress_mgr * progress
void * client_data
boolean is_decompressor
int global_state
struct jpeg_destination_mgr * dest
JDIMENSION image_width
JDIMENSION image_height
int input_components
J_COLOR_SPACE in_color_space
double input_gamma
int data_precision
int num_components
J_COLOR_SPACE jpeg_color_space
jpeg_component_info * comp_info
JQUANT_TBL * quant_tbl_ptrs [4]
JHUFF_TBL * dc_huff_tbl_ptrs [4]
JHUFF_TBL * ac_huff_tbl_ptrs [4]
UINT8 arith_dc_L [16]
UINT8 arith_dc_U [16]
UINT8 arith_ac_K [16]
int num_scans
const jpeg_scan_info * scan_info
boolean raw_data_in
boolean arith_code
boolean optimize_coding
boolean CCIR601_sampling
int smoothing_factor
J_DCT_METHOD dct_method
unsigned int restart_interval
int restart_in_rows
boolean write_JFIF_header
UINT8 JFIF_major_version
UINT8 JFIF_minor_version
UINT8 density_unit
UINT16 X_density
UINT16 Y_density
boolean write_Adobe_marker
JDIMENSION next_scanline
boolean progressive_mode
int max_h_samp_factor
int max_v_samp_factor
JDIMENSION total_iMCU_rows
int comps_in_scan
jpeg_component_info * cur_comp_info [4]
JDIMENSION MCUs_per_row
JDIMENSION MCU_rows_in_scan
int blocks_in_MCU
int MCU_membership [10]
int Ss
int Se
int Ah
int Al
struct jpeg_comp_master * master
struct jpeg_c_main_controller * main
struct jpeg_c_prep_controller * prep
struct jpeg_c_coef_controller * coef
struct jpeg_marker_writer * marker
struct jpeg_color_converter * cconvert
struct jpeg_downsampler * downsample
struct jpeg_forward_dct * fdct
struct jpeg_entropy_encoder * entropy
jpeg_scan_info * script_space
int script_space_size
> This is bizarre and truly beyond me. The warning is saying something is wrong. Covering your eyes doesn't make the boogie man go away. Either the compiler is broken (please show me - I wouldn't deny it) or the code is wrong.
In fact, I think that it's neither. GCC warns you in case you wanted to initialize with something other than 0, but it should be dutifully zero initializing the whole structure.
This kind of makes sense for { 0 } (what if you did want to initialize with another value indeed?), but hardly for {} - in that case, the intention couldn't be clearer.
> It's a C library so there is no constructor.
I'm not so sure. I think that a default constructor will zero initialize, as long as you invoke it explicitly (something like "jpeg_compress_struct compressData()" or "jpeg_compress_struct compressData = jpeg_compress_struct()"). It's just an issue of backwards compatibility that it's not invoked for "jpeg_compress_struct compressData".
I'm not 100% confident about this though.
Comment on attachment 131488[details]
WebCore part of signed/unsigned mismatch warnings, improved
View in context: https://bugs.webkit.org/attachment.cgi?id=131488&action=review
r=me assuming you verify that the comparison in IconDatabase is not bogus.
> Source/WebCore/loader/icon/IconDatabase.cpp:655
> - return (int)currentTime() - icon->getTimestamp() > iconExpirationTime ? IconLoadYes : IconLoadNo;
> + return static_cast<int>(currentTime()) - static_cast<int>(icon->getTimestamp()) > iconExpirationTime ? IconLoadYes : IconLoadNo;
Can we cast the other way to avoid trouble in 2038, and for general sanity? currentTime() returns a double.
Actually, this looks slightly suspicious - do we know that these use the same epoch on all platforms?
(In reply to comment #37)
> (From update of attachment 131488[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131488&action=review
>
> r=me assuming you verify that the comparison in IconDatabase is not bogus.
>
> > Source/WebCore/loader/icon/IconDatabase.cpp:655
> > - return (int)currentTime() - icon->getTimestamp() > iconExpirationTime ? IconLoadYes : IconLoadNo;
> > + return static_cast<int>(currentTime()) - static_cast<int>(icon->getTimestamp()) > iconExpirationTime ? IconLoadYes : IconLoadNo;
>
> Can we cast the other way to avoid trouble in 2038, and for general sanity? currentTime() returns a double.
>
> Actually, this looks slightly suspicious - do we know that these use the same epoch on all platforms?
Oops I just noticed this comment after sending to CQ. I think the question is, do we have a test for this code? No regressions were noticed with this code on QNX.
> Oops I just noticed this comment after sending to CQ. I think the question is, do we have a test for this code? No regressions were noticed with this code on QNX.
You could still stop CQ, it double-checks the flag right before committing.
I doubt that there is any test coverage for this.
Comment on attachment 131704[details]
Fix cast-align GCC warning
View in context: https://bugs.webkit.org/attachment.cgi?id=131704&action=review> Source/WebCore/ChangeLog:9
> + (WebCore):
prepare-ChangeLog didn't pick up unpackOneRowOfBGRA8ToRGBA8() (its in an anonymous namespace). For your consideration, I suggest adding a remark that you modified unpackOneRowOfBGRA8ToRGBA8() so as to make it easier to search for changes to this function on trac.webkit.org and using VCS tools.
(In reply to comment #43)
> (From update of attachment 131704[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131704&action=review
>
> > Source/WebCore/ChangeLog:9
> > + (WebCore):
>
> prepare-ChangeLog didn't pick up unpackOneRowOfBGRA8ToRGBA8() (its in an anonymous namespace). For your consideration, I suggest adding a remark that you modified unpackOneRowOfBGRA8ToRGBA8() so as to make it easier to search for changes to this function on trac.webkit.org and using VCS tools.
Thanks Dan, this ultimately landed in r110602 with a comment in the ChangeLog.
Comment on attachment 131839[details]
Fix cast-align GCC warnings
This is ARM/MIPS specific. You could say that in the ChangeLog, only then reinterpret_cast_ptr and reinterpret_cast differ. r=me.
Comment on attachment 131253[details]
Correct usage of NDEBUG to avoid warnings on BlackBerry
Rejecting attachment 131253[details] from commit-queue.
Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
Last 500 characters of output:
Source/WebCore/platform/blackberry/CookieManager.cpp.rej
patching file Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.cpp
Hunk #1 FAILED at 416.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.cpp.rej
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Nikolas Zi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/
Full output: http://queues.webkit.org/results/11968177
Comment on attachment 131253[details]
Correct usage of NDEBUG to avoid warnings on BlackBerry
Rejecting attachment 131253[details] from commit-queue.
Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
Last 500 characters of output:
Source/WebCore/platform/blackberry/CookieManager.cpp.rej
patching file Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.cpp
Hunk #1 FAILED at 416.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.cpp.rej
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Nikolas Zi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/
Full output: http://queues.webkit.org/results/11984758
Comment on attachment 131372[details]
Warnings - Harfbuzz const return type doesn't make sense
Rejecting attachment 131372[details] from commit-queue.
Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
Last 500 characters of output:
.
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.h
Hunk #1 FAILED at 83.
Hunk #2 FAILED at 91.
2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.h.rej
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Rob Buis']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/
Full output: http://queues.webkit.org/results/12007258
(In reply to comment #34)
> (In reply to comment #33)
> > I'd suggest either initializing each member, or silencing down this warning for gcc.
>
> This is bizarre and truly beyond me. The warning is saying something is wrong. Covering your eyes doesn't make the boogie man go away. Either the compiler is broken (please show me - I wouldn't deny it) or the code is wrong. If the code is wrong, initializing each member is the most error-prone choice of solution.
Perhaps the only wrong here is an over-zealous gcc. libjpeg internally memset()'s the various structures automatically when you start a compress, or start a decompress.
Comment on attachment 142909[details]
Fix warning in JPEG decoder
Rejecting attachment 142909[details] from commit-queue.
Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
Last 500 characters of output:
ly', u'--force', u'--reviewer', u'Rob Buis']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue/
Parsed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
patch: **** malformed patch at line 18: Simplify RenderOverflow by using Rects
patching file Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Rob Buis']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue/
Full output: http://queues.webkit.org/results/12727894
Comment on attachment 142909[details]
Fix warning in JPEG decoder
Rejecting attachment 142909[details] from commit-queue.
Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
Last 500 characters of output:
ly', u'--force', u'--reviewer', u'Rob Buis']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue/
Parsed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
patch: **** malformed patch at line 18: Simplify RenderOverflow by using Rects
patching file Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Rob Buis']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue/
Full output: http://queues.webkit.org/results/12739251
The commit-queue encountered the following flaky tests while processing attachment 201099[details]:
svg/as-image/img-preserveAspectRatio-support-2.html bug 114434 (author: zimmermann@kde.org)
The commit-queue is continuing to process your patch.
2012-03-11 09:48 PDT, George Staikos
2012-03-11 09:49 PDT, George Staikos
2012-03-11 09:51 PDT, George Staikos
2012-03-11 09:52 PDT, George Staikos
2012-03-11 09:53 PDT, George Staikos
2012-03-11 09:55 PDT, George Staikos
2012-03-11 09:56 PDT, George Staikos
2012-03-11 10:00 PDT, George Staikos
2012-03-11 11:59 PDT, George Staikos
ap: commit-queue-
2012-03-11 12:05 PDT, George Staikos
2012-03-11 12:10 PDT, George Staikos
2012-03-11 12:13 PDT, George Staikos
2012-03-12 11:57 PDT, George Staikos
2012-03-12 19:07 PDT, George Staikos
2012-03-13 13:29 PDT, Rob Buis
2012-03-14 07:22 PDT, Rob Buis
2012-03-16 10:42 PDT, Rob Buis
2012-03-20 08:30 PDT, Rob Buis
2012-04-12 01:59 PDT, George Staikos
2012-04-13 10:49 PDT, Rob Buis
2012-05-20 12:05 PDT, George Staikos
2012-05-20 17:20 PDT, George Staikos
2012-08-31 07:57 PDT, Rob Buis
2013-05-08 12:58 PDT, Rob Buis
2013-05-08 13:12 PDT, Rob Buis
2013-05-09 08:50 PDT, Jacky Jiang
2013-05-10 14:07 PDT, Jacky Jiang