Bug 38073 – Clean up a few compiler warnings seen on Chromium Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Clean up a few compiler warnings

[James Robinson]

Created attachment 54208 [details]
Patch

[Darin Adler]

Comment on attachment 54208 [details]
Patch

What compiler generates these warnings?

[Darin Adler]

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).

[Darin Adler]

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?

[James Robinson]

Created attachment 54330 [details]
Patch

[James Robinson]

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.

[Darin Adler]

(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.

[Eric Seidel (no email)]

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.

[James Robinson]

Created attachment 54370 [details]
Patch

[James Robinson]

I like Eric's idea, this patch does that.  It adds a FIXME instead of the cast.

[Darin Adler]

(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.

[James Robinson]

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).

[David Levin]

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.

[Eric Seidel (no email)]

Comment on attachment 54370 [details]
Patch

LGTM.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Attachment 54370 [details] was posted by a committer and has review+, assigning to James Robinson for commit.

[James Robinson]

Comment on attachment 54370 [details]
Patch

Clearing flags on attachment: 54370

Committed r58718: <http://trac.webkit.org/changeset/58718>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[Evan Martin]

I filed bug 51566 for the discussion of the mysterious truncator unsigned width() from comments #4, #7, #8, #9.