Bug 80790 - Fix some compiler warnings (miscellaneous)
Summary: Fix some compiler warnings (miscellaneous)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: George Staikos
URL:
Keywords:
: 80538 80544 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-03-11 09:46 PDT by George Staikos
Modified: 2013-05-10 14:28 PDT (History)
8 users (show)

See Also:


Attachments
Warnings - IconDatabase (1.30 KB, patch)
2012-03-11 09:48 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warnings - AssemblerBuffer (1.25 KB, patch)
2012-03-11 09:49 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warnings - MacroAssemblerArm (1.25 KB, patch)
2012-03-11 09:51 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warnings - Lexer (928 bytes, patch)
2012-03-11 09:52 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warning - Unused global static for BlackBerry (1.10 KB, patch)
2012-03-11 09:53 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warning - MimeSniffing (1.13 KB, patch)
2012-03-11 09:55 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warnings - BlackBerry cookies (2.10 KB, patch)
2012-03-11 09:56 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warnings - Properly Initialize JPEG structure (1.29 KB, patch)
2012-03-11 10:00 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warnings - Signed/Unsigned mismatches (merged) (4.75 KB, patch)
2012-03-11 11:59 PDT, George Staikos
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Remove unused global static for BlackBerry (1.25 KB, patch)
2012-03-11 12:05 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Correct usage of NDEBUG to avoid warnings on BlackBerry (2.12 KB, patch)
2012-03-11 12:10 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Properly initialize JPEG structure (1.44 KB, patch)
2012-03-11 12:13 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Warnings - Harfbuzz const return type doesn't make sense (2.14 KB, patch)
2012-03-12 11:57 PDT, George Staikos
no flags Details | Formatted Diff | Diff
WebCore part of signed/unsigned mismatch warnings, improved (2.07 KB, patch)
2012-03-12 19:07 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Fix cast-align GCC warning (1.58 KB, patch)
2012-03-13 13:29 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Fix cast-align GCC warnings (2.37 KB, patch)
2012-03-14 07:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Fix cast-align warnings in JSC (8.72 KB, patch)
2012-03-16 10:42 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (12.63 KB, patch)
2012-03-20 08:30 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Fix a warning in cookies code (1.33 KB, patch)
2012-04-12 01:59 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Patch (1.22 KB, patch)
2012-04-13 10:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Fix warning in JPEG decoder (1.20 KB, patch)
2012-05-20 12:05 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Fix warning in jpeg encoder (1.21 KB, patch)
2012-05-20 17:20 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Patch (1.61 KB, patch)
2012-08-31 07:57 PDT, Rob Buis
darin: review+
Details | Formatted Diff | Diff
Patch (2.02 KB, patch)
2013-05-08 12:58 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.63 KB, patch)
2013-05-08 13:12 PDT, Rob Buis
pdr: review+
Details | Formatted Diff | Diff
Patch (6.76 KB, patch)
2013-05-09 08:50 PDT, Jacky Jiang
no flags Details | Formatted Diff | Diff
Patch (5.12 KB, patch)
2013-05-10 14:07 PDT, Jacky Jiang
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Staikos 2012-03-11 09:46:10 PDT
Patches to come - mostly signed/unsigned changes.
Comment 1 George Staikos 2012-03-11 09:48:49 PDT
Created attachment 131230 [details]
Warnings - IconDatabase
Comment 2 George Staikos 2012-03-11 09:49:58 PDT
Created attachment 131231 [details]
Warnings - AssemblerBuffer
Comment 3 George Staikos 2012-03-11 09:51:26 PDT
Created attachment 131232 [details]
Warnings - MacroAssemblerArm
Comment 4 George Staikos 2012-03-11 09:52:28 PDT
Created attachment 131233 [details]
Warnings - Lexer
Comment 5 George Staikos 2012-03-11 09:53:43 PDT
Created attachment 131234 [details]
Warning - Unused global static for BlackBerry
Comment 6 George Staikos 2012-03-11 09:55:23 PDT
Created attachment 131235 [details]
Warning - MimeSniffing
Comment 7 George Staikos 2012-03-11 09:56:29 PDT
Created attachment 131236 [details]
Warnings - BlackBerry cookies
Comment 8 George Staikos 2012-03-11 10:00:27 PDT
Created attachment 131237 [details]
Warnings - Properly Initialize JPEG structure

This jpeg patch is possibly a real bug also.
Comment 9 George Staikos 2012-03-11 10:41:42 PDT
Niko, I just uploaded the wrong set of patches - no changelogs.
Comment 10 Nikolas Zimmermann 2012-03-11 10:52:12 PDT
Oops, it's too late, I didn't see that there were missing ChangeLogs :-)
Comment 11 George Staikos 2012-03-11 11:59:34 PDT
Created attachment 131251 [details]
Warnings - Signed/Unsigned mismatches (merged)
Comment 12 George Staikos 2012-03-11 12:05:37 PDT
Created attachment 131252 [details]
Remove unused global static for BlackBerry
Comment 13 George Staikos 2012-03-11 12:10:59 PDT
Created attachment 131253 [details]
Correct usage of NDEBUG to avoid warnings on BlackBerry
Comment 14 George Staikos 2012-03-11 12:13:31 PDT
Created attachment 131254 [details]
Properly initialize JPEG structure

This may be a real bug also - not sure.
Comment 15 Alexey Proskuryakov 2012-03-11 21:18:15 PDT
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).
Comment 16 Alexey Proskuryakov 2012-03-11 21:20:01 PDT
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?
Comment 17 George Staikos 2012-03-12 05:59:29 PDT
(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.
Comment 18 George Staikos 2012-03-12 06:06:14 PDT
(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 19 WebKit Review Bot 2012-03-12 08:29:07 PDT
Comment on attachment 131253 [details]
Correct usage of NDEBUG to avoid warnings on BlackBerry

Clearing flags on attachment: 131253

Committed r110428: <http://trac.webkit.org/changeset/110428>
Comment 20 WebKit Review Bot 2012-03-12 08:33:05 PDT
Comment on attachment 131252 [details]
Remove unused global static for BlackBerry

Clearing flags on attachment: 131252

Committed r110429: <http://trac.webkit.org/changeset/110429>
Comment 21 Alexey Proskuryakov 2012-03-12 10:29:48 PDT
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.
Comment 22 Alexey Proskuryakov 2012-03-12 10:33:33 PDT
>   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.
Comment 23 George Staikos 2012-03-12 11:57:39 PDT
Created attachment 131372 [details]
Warnings - Harfbuzz const return type doesn't make sense
Comment 24 George Staikos 2012-03-12 12:01:28 PDT
(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.
Comment 25 Alexey Proskuryakov 2012-03-12 13:24:26 PDT
Comment on attachment 131254 [details]
Properly initialize JPEG structure

Clearing flags for now, please investigate suggestion in comment 21.
Comment 26 George Staikos 2012-03-12 13:48:26 PDT
(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 27 WebKit Review Bot 2012-03-12 14:36:31 PDT
Comment on attachment 131372 [details]
Warnings - Harfbuzz const return type doesn't make sense

Clearing flags on attachment: 131372

Committed r110481: <http://trac.webkit.org/changeset/110481>
Comment 28 WebKit Review Bot 2012-03-12 14:37:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 George Staikos 2012-03-12 16:04:31 PDT
two patches left to deal with.
Comment 30 George Staikos 2012-03-12 16:05:14 PDT
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.
Comment 31 Alexey Proskuryakov 2012-03-12 16:29:10 PDT
Well, as mentioned above, memset is not rigorously correct, so there must be a better solution.
Comment 32 George Staikos 2012-03-12 17:02:52 PDT
(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.
Comment 33 Alexey Proskuryakov 2012-03-12 17:09:29 PDT
I'd suggest either initializing each member, or silencing down this warning for gcc.
Comment 34 George Staikos 2012-03-12 17:21:53 PDT
(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
Comment 35 George Staikos 2012-03-12 19:07:27 PDT
Created attachment 131488 [details]
WebCore part of signed/unsigned mismatch warnings, improved
Comment 36 Alexey Proskuryakov 2012-03-12 22:41:22 PDT
> 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 37 Alexey Proskuryakov 2012-03-12 22:54:27 PDT
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?
Comment 38 George Staikos 2012-03-13 07:15:36 PDT
(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.
Comment 39 WebKit Review Bot 2012-03-13 08:24:19 PDT
Comment on attachment 131488 [details]
WebCore part of signed/unsigned mismatch warnings, improved

Clearing flags on attachment: 131488

Committed r110566: <http://trac.webkit.org/changeset/110566>
Comment 40 Alexey Proskuryakov 2012-03-13 08:57:49 PDT
> 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 41 Rob Buis 2012-03-13 09:24:38 PDT
*** Bug 80538 has been marked as a duplicate of this bug. ***
Comment 42 Rob Buis 2012-03-13 13:29:55 PDT
Created attachment 131704 [details]
Fix cast-align GCC warning
Comment 43 Daniel Bates 2012-03-13 13:51:26 PDT
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.
Comment 44 Rob Buis 2012-03-13 14:30:37 PDT
(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 45 Rob Buis 2012-03-13 15:06:54 PDT
*** Bug 80544 has been marked as a duplicate of this bug. ***
Comment 46 Rob Buis 2012-03-14 07:22:49 PDT
Created attachment 131839 [details]
Fix cast-align GCC warnings
Comment 47 Nikolas Zimmermann 2012-03-15 07:00:18 PDT
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 48 Nikolas Zimmermann 2012-03-15 07:01:15 PDT
Comment on attachment 131253 [details]
Correct usage of NDEBUG to avoid warnings on BlackBerry

r=me on the NDEBUG patch.
Comment 49 Rob Buis 2012-03-15 07:37:09 PDT
cast-align warnings fix landed in r110844.
Comment 50 WebKit Review Bot 2012-03-16 10:16:02 PDT
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 51 Rob Buis 2012-03-16 10:42:08 PDT
Created attachment 132317 [details]
Fix cast-align warnings in JSC
Comment 52 George Staikos 2012-03-19 14:35:08 PDT
Comment on attachment 131253 [details]
Correct usage of NDEBUG to avoid warnings on BlackBerry

Trying again
Comment 53 WebKit Review Bot 2012-03-19 14:38:27 PDT
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 54 Rob Buis 2012-03-20 06:53:05 PDT
Comment on attachment 131372 [details]
Warnings - Harfbuzz const return type doesn't make sense

Looks good.
Comment 55 WebKit Review Bot 2012-03-20 06:56:15 PDT
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
Comment 56 Rob Buis 2012-03-20 07:17:00 PDT
Comment on attachment 131488 [details]
WebCore part of signed/unsigned mismatch warnings, improved

Already landed.
Comment 57 Rob Buis 2012-03-20 08:30:22 PDT
Created attachment 132830 [details]
Patch
Comment 58 Eric Seidel (no email) 2012-03-27 12:51:02 PDT
Comment on attachment 131253 [details]
Correct usage of NDEBUG to avoid warnings on BlackBerry

Cleared Nikolas Zimmermann's review+ from obsolete attachment 131253 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 59 Eric Seidel (no email) 2012-03-27 12:51:07 PDT
Comment on attachment 131372 [details]
Warnings - Harfbuzz const return type doesn't make sense

Cleared Rob Buis's review+ from obsolete attachment 131372 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 60 Eric Seidel (no email) 2012-03-27 12:51:12 PDT
Comment on attachment 131839 [details]
Fix cast-align GCC warnings

Cleared Nikolas Zimmermann's review+ from obsolete attachment 131839 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 61 noel gordon 2012-04-12 01:12:24 PDT
(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 62 George Staikos 2012-04-12 01:59:40 PDT
Created attachment 136856 [details]
Fix a warning in cookies code
Comment 63 Rob Buis 2012-04-12 04:51:14 PDT
Comment on attachment 136856 [details]
Fix a warning in cookies code

LGTM.
Comment 64 Rob Buis 2012-04-13 10:49:28 PDT
Created attachment 137102 [details]
Patch
Comment 65 George Staikos 2012-04-15 23:51:54 PDT
Comment on attachment 136856 [details]
Fix a warning in cookies code

I dont' see this patch in, despite apparently having gone through commit queue?
Comment 66 WebKit Review Bot 2012-04-16 01:11:45 PDT
Comment on attachment 136856 [details]
Fix a warning in cookies code

Clearing flags on attachment: 136856

Committed r114230: <http://trac.webkit.org/changeset/114230>
Comment 67 Eric Seidel (no email) 2012-04-19 16:58:07 PDT
Comment on attachment 132830 [details]
Patch

Cleared George Staikos's review+ from obsolete attachment 132830 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 68 George Staikos 2012-05-20 12:05:11 PDT
Created attachment 142909 [details]
Fix warning in JPEG decoder
Comment 69 Rob Buis 2012-05-20 12:56:56 PDT
Comment on attachment 142909 [details]
Fix warning in JPEG decoder

Looks good.
Comment 70 WebKit Review Bot 2012-05-20 16:14:13 PDT
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 71 George Staikos 2012-05-20 17:03:14 PDT
Comment on attachment 142909 [details]
Fix warning in JPEG decoder

Trying CQ one more time....
Comment 72 WebKit Review Bot 2012-05-20 17:04:47 PDT
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
Comment 73 George Staikos 2012-05-20 17:20:05 PDT
Created attachment 142927 [details]
Fix warning in jpeg encoder

Fixes the patch format so CQ can run it.
Comment 74 WebKit Review Bot 2012-05-20 17:51:17 PDT
Comment on attachment 142927 [details]
Fix warning in jpeg encoder

Clearing flags on attachment: 142927

Committed r117718: <http://trac.webkit.org/changeset/117718>
Comment 75 WebKit Review Bot 2012-07-27 05:03:51 PDT
Comment on attachment 142909 [details]
Fix warning in JPEG decoder

Cleared Rob Buis's review+ from obsolete attachment 142909 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 76 Rob Buis 2012-08-31 07:08:44 PDT
Comment on attachment 136856 [details]
Fix a warning in cookies code

This change landed at some point
Comment 77 Rob Buis 2012-08-31 07:57:33 PDT
Created attachment 161699 [details]
Patch
Comment 78 Rob Buis 2013-05-08 12:58:12 PDT
Created attachment 201099 [details]
Patch
Comment 79 Rob Buis 2013-05-08 13:12:25 PDT
Created attachment 201100 [details]
Patch
Comment 80 Rob Buis 2013-05-08 13:37:14 PDT
Comment on attachment 201100 [details]
Patch

Landed in r149763.
Comment 81 Brent Fulgham 2013-05-08 23:05:23 PDT
Comment on attachment 201099 [details]
Patch

r=me
Comment 82 WebKit Commit Bot 2013-05-08 23:37:17 PDT
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.
Comment 83 WebKit Commit Bot 2013-05-08 23:38:40 PDT
Comment on attachment 201099 [details]
Patch

Clearing flags on attachment: 201099

Committed r149794: <http://trac.webkit.org/changeset/149794>
Comment 84 WebKit Commit Bot 2013-05-08 23:38:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 85 Jacky Jiang 2013-05-09 08:50:26 PDT
Reopening to attach new patch.
Comment 86 Jacky Jiang 2013-05-09 08:50:33 PDT
Created attachment 201243 [details]
Patch
Comment 87 Rob Buis 2013-05-09 08:56:00 PDT
Comment on attachment 201243 [details]
Patch

LGTM.
Comment 88 Jacky Jiang 2013-05-09 09:03:28 PDT
Comment on attachment 201243 [details]
Patch

I am committing myself.
Comment 89 Jacky Jiang 2013-05-09 09:09:54 PDT
Committed r149816: <http://trac.webkit.org/changeset/149816>
Comment 90 Jacky Jiang 2013-05-10 14:07:30 PDT
Reopening to attach new patch.
Comment 91 Jacky Jiang 2013-05-10 14:07:37 PDT
Created attachment 201423 [details]
Patch
Comment 92 Rob Buis 2013-05-10 14:18:01 PDT
Comment on attachment 201423 [details]
Patch

LGTM.
Comment 93 Jacky Jiang 2013-05-10 14:28:44 PDT
Committed r149907: <http://trac.webkit.org/changeset/149907>