Bug 27851 - mutil-char constant warning in BMPImageDecoder.cpp under gcc
Summary: mutil-char constant warning in BMPImageDecoder.cpp under gcc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-30 12:33 PDT by Xan Lopez
Modified: 2009-08-05 14:14 PDT (History)
3 users (show)

See Also:


Attachments
bmpwarning.patch (1.96 KB, patch)
2009-07-30 12:33 PDT, Xan Lopez
eric: review-
Details | Formatted Diff | Diff
Fix two different warnings (30.52 KB, patch)
2009-08-05 14:04 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Fix two different warnings (correct patch) (2.46 KB, patch)
2009-08-05 14:06 PDT, Peter Kasting
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2009-07-30 12:33:16 PDT
Created attachment 33803 [details]
bmpwarning.patch

GCC does not like multi-character character constants, so it produces the following warning for BMPImageDecoder.cpp:

  CXX    WebCore/platform/image-decoders/bmp/libWebCore_la-BMPImageDecoder.lo
../../WebCore/platform/image-decoders/bmp/BMPImageDecoder.cpp:128:16: warning: multi-character character constant

The attached patch fixes the warning, but since this might be a bit controversial *and* I can't run the tests to verify that this does not break anything (they all fail with 'timed out' now, something must be broken somewhere...), I'm opening a bug for this.
Comment 1 Adam Barth 2009-07-30 18:28:16 PDT
Why do you think this is controversial?
Comment 2 Xan Lopez 2009-07-30 22:53:39 PDT
(In reply to comment #1)
> Why do you think this is controversial?

Well, at least for me it's not 100% obvious that this is correct, won't break anything, that the old code is not somehow relying on the use of multi-character character constants...

That being said the value of multi-character character constants is not guaranteed to be the same across platforms (according to Google), so I think a fix similar to this one (if not this one) should be applied.
Comment 3 Xan Lopez 2009-07-31 05:23:37 PDT
I landed this as r46626, closing.
Comment 4 Xan Lopez 2009-07-31 05:59:19 PDT
If you believe the bots this caused:

- A crash in GTK+ in test svg/dom/SVGScriptElement/script-load-and-error-events.svg (it works fine in my box).
- A layout failure in Qt for test animations/change-keyframes.html.
- A crash in Windows Debug for test http/tests/security/aboutBlank/xss-DENIED-navigate-opener-document-write.html (!?).

So I've rolled it out.
Comment 5 Eric Seidel (no email) 2009-07-31 21:27:14 PDT
Comment on attachment 33803 [details]
bmpwarning.patch

rolled out, marking r-
Comment 6 Peter Kasting 2009-08-05 14:04:40 PDT
Created attachment 34171 [details]
Fix two different warnings

The original patch was sound.  I think the test failures encountered must have been due to flakiness or another change.  Verified that this still decodes BMPs OK.

I also fixed another warning I just introduced (sorry!).
Comment 7 Peter Kasting 2009-08-05 14:06:01 PDT
Created attachment 34172 [details]
Fix two different warnings (correct patch)

Oops, attached wrong patch.  I have too many patches in flight :(
Comment 8 Peter Kasting 2009-08-05 14:14:37 PDT
Fixed in r46814.