Bug 38073 - Clean up a few compiler warnings seen on Chromium Windows
Summary: Clean up a few compiler warnings seen on Chromium Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-23 18:37 PDT by James Robinson
Modified: 2010-12-23 15:22 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.10 KB, patch)
2010-04-23 18:38 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2010-04-26 14:21 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (3.66 KB, patch)
2010-04-26 19:09 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2010-04-23 18:37:00 PDT
Clean up a few compiler warnings
Comment 1 James Robinson 2010-04-23 18:38:52 PDT
Created attachment 54208 [details]
Patch
Comment 2 Darin Adler 2010-04-24 09:27:44 PDT
Comment on attachment 54208 [details]
Patch

What compiler generates these warnings?
Comment 3 Darin Adler 2010-04-24 09:28:34 PDT
Comment on attachment 54208 [details]
Patch

>  class StyleReflection;
>  class StyleTransformData;
>  struct ContentData;
> -struct ShadowData;
> +class ShadowData;

This should be re-sorted above with the class forward declarations. And the struct forward declarations should be in a separate paragraph (blank line between the paragraphs).
Comment 4 Darin Adler 2010-04-24 09:29:27 PDT
Comment on attachment 54208 [details]
Patch

> -    unsigned width() const { return m_width; }
> +    unsigned width() const { return static_cast<unsigned>(m_width); }
>      void setWidth(float w) { m_width = w; }

This worries me; not the type cast, but the existing code. Why is it OK to convert the width into an unsigned?
Comment 5 James Robinson 2010-04-26 14:21:12 PDT
Created attachment 54330 [details]
Patch
Comment 6 James Robinson 2010-04-26 14:22:04 PDT
The warnings are from MSVC, but they are all fairly general.  I agree that the TextMetrics rounding looks weird, but I'm not familiar enough with this code to guess whether or not it's an issue.
Comment 7 Darin Adler 2010-04-26 14:48:20 PDT
(In reply to comment #6)
> The warnings are from MSVC

Good to know.

Are these particular MSVC warnings off for the Windows build (the one Apple uses for Windows Safari)? Should we turn them on?

> but they are all fairly general.

I don't know what that means.

>  I agree that the
> TextMetrics rounding looks weird, but I'm not familiar enough with this code to
> guess whether or not it's an issue.

Generally I think it’s not good to silence a warning if the warning may be pointed to a real problem. I am familiar enough to know that it might be an issue.
Comment 8 Eric Seidel (no email) 2010-04-26 16:53:07 PDT
Comment on attachment 54330 [details]
Patch

I recommend posting and landing the uncontentious fixes first, and then asking mitz about what's up with the width() code.
Comment 9 James Robinson 2010-04-26 19:09:55 PDT
Created attachment 54370 [details]
Patch
Comment 10 James Robinson 2010-04-26 19:10:53 PDT
I like Eric's idea, this patch does that.  It adds a FIXME instead of the cast.
Comment 11 Darin Adler 2010-04-27 08:22:33 PDT
(In reply to comment #7)
> Are these particular MSVC warnings off for the Windows build (the one Apple
> uses for Windows Safari)? Should we turn them on?

I'm still waiting for the answer to this question.
Comment 12 James Robinson 2010-04-27 16:34:51 PDT
I should have clarified that these are warnings seen in the Chromium Windows build using those warning settings.  I don't know which warnings the Safari Win port suppresses and do not have access to a Safari Win build environment to check.  I think those decisions will have to be made by the Safari Win port maintainers.  That said, I think it's reasonable that shared code not generate warnings for any port unless there's a good reason to (which there does not seem to be in this case).
Comment 13 David Levin 2010-04-27 17:58:43 PDT
s/Safari/WebKit/

(In reply to comment #12)
> I should have clarified that these are warnings seen in the Chromium Windows
> build using those warning settings.  I don't know which warnings the Safari Win
> port suppresses and do not have access to a Safari Win build environment to
> check.

Here ya go: http://webkit.org/building/checkout.html

> I think those decisions will have to be made by the Safari Win port
> maintainers.

I bet folks would be ok with the warnings being turned on as long as it doesn't break the build. If in doubt, you could send email to webkit-dev asking for objections.

By turning on the warning, you will help make sure that more people are concerned about them as well.
Comment 14 Eric Seidel (no email) 2010-05-02 19:01:31 PDT
Comment on attachment 54370 [details]
Patch

LGTM.
Comment 15 Eric Seidel (no email) 2010-05-02 19:03:20 PDT
Adam, Steve, and Brian are a few of the windows guys.  If you post a patch to WebCore.vcproj enabling the warnings, I expect they'd be happy to review.
Comment 16 Eric Seidel (no email) 2010-05-02 19:32:06 PDT
Attachment 54370 [details] was posted by a committer and has review+, assigning to James Robinson for commit.
Comment 17 James Robinson 2010-05-03 17:04:15 PDT
Comment on attachment 54370 [details]
Patch

Clearing flags on attachment: 54370

Committed r58718: <http://trac.webkit.org/changeset/58718>
Comment 18 James Robinson 2010-05-03 17:04:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Evan Martin 2010-12-23 15:22:05 PST
I filed bug 51566 for the discussion of the mysterious truncator unsigned width() from comments #4, #7, #8, #9.