Open Bug 832788 Opened 7 years ago Updated 7 months ago

Fix MSVC build warnings in dom/bindings

Categories

(Core :: DOM: Core & HTML, defect)

18 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

People

(Reporter: roc, Unassigned)

Details

(Whiteboard: [leave open])

Attachments

(6 files, 2 obsolete files)

5.24 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
743 bytes, patch
ehsan
: review+
Details | Diff | Splinter Review
1.42 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.90 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.44 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
727 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
MSVC produces a bunch of warnings about narrowing conversions.
Comment on attachment 704360 [details] [diff] [review]
Change intermediateType to float to avoid build warni ngs in MSVC

This is wrong.
Attachment #704360 - Attachment is obsolete: true
Attachment #704360 - Flags: review?(bzbarsky)
Summary: Fix building warnings in PrimitiveConversionTraits_float → Fix MSVC build warnings in dom/bindings
Comment on attachment 704403 [details] [diff] [review]
Part 4: PrimitiveConversions should define intermediateType to avoid implicit narrowing conversions

Could we get the same effect simply by doing an explicit cast to T after casting to intermediateType in ValueToPrimitive?
Attachment #704401 - Flags: review?(ehsan) → review+
Attachment #704402 - Flags: review?(ehsan) → review+
(In reply to Boris Zbarsky (:bz) from comment #7)
> Could we get the same effect simply by doing an explicit cast to T after
> casting to intermediateType in ValueToPrimitive?

I'm sure we could. Is that better?
I think I'd somewhat prefer that, yeah.
Comment on attachment 704766 [details] [diff] [review]
Part 4: PrimitiveConversions should avoid implicit narrowing casts to avoid compiler warnings; make them explicit

r=me
Attachment #704766 - Flags: review?(bzbarsky) → review+
Comment on attachment 704399 [details] [diff] [review]
Part 1: Make double->float conversions explicit in CanvasRenderingContext2D.cpp

Review of attachment 704399 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable.
Attachment #704399 - Flags: review?(jmuizelaar) → review+
Comment on attachment 707506 [details] [diff] [review]
Part 5: Add suffixes for large integer literals

r=me
Attachment #707506 - Flags: review?(bzbarsky) → review+
Comment on attachment 707507 [details] [diff] [review]
Part 6: Enable FAIL_ON_WARNINGS in dom/bindings

Unfortunately, I think this will break the build for anyone using clang and ccache (e.g. me) because that configuration warns all over the place on some XPConnect headers that binding code includes...  I can try the patch locally if you want me to, I guess, and see if it compiles.
Though bug 831885 might fix that problem, actually...
(In reply to comment #18)
> Comment on attachment 707507 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=707507
> Part 6: Enable FAIL_ON_WARNINGS in dom/bindings
> 
> Unfortunately, I think this will break the build for anyone using clang and
> ccache (e.g. me) because that configuration warns all over the place on some
> XPConnect headers that binding code includes...  I can try the patch locally if
> you want me to, I guess, and see if it compiles.

Hmm, I don't get those warnings here...
> Hmm, I don't get those warnings here...

I'm talking about these ones:

../../../mozilla/js/xpconnect/src/XPCMaps.h:489:31: warning: equality comparison with extraneous parentheses [-Wparentheses-equality]
        if (((entry)->keyHash == 0))
             ~~~~~~~~~~~~~~~~~^~~~
../../../mozilla/js/xpconnect/src/XPCMaps.h:489:31: note: remove extraneous parentheses around the comparison to silence this warning
        if (((entry)->keyHash == 0))
            ~                 ^   ~
../../../mozilla/js/xpconnect/src/XPCMaps.h:489:31: note: use '=' to turn this equality comparison into an assignment
        if (((entry)->keyHash == 0))
                              ^~
                              =
(In reply to Boris Zbarsky (:bz) from comment #18)
> Comment on attachment 707507 [details] [diff] [review]
> Part 6: Enable FAIL_ON_WARNINGS in dom/bindings
> 
> Unfortunately, I think this will break the build for anyone using clang and
> ccache (e.g. me) because that configuration warns all over the place on some
> XPConnect headers that binding code includes...  I can try the patch locally
> if you want me to, I guess, and see if it compiles.

Do you actually opt into --enable-warnings-as-errors? And do those warnings become fatal?

These patches were green on try ... even though the logs still show warnings about possibly-uninitialized on some platforms. I assume those warnings are not errors even with FAIL_ON_WARNINGS.
I think that if your configuration produces errors on code that's green on tinderbox, --enable-warnings-as-errors is a bad idea for you :-).
Comment on attachment 707507 [details] [diff] [review]
Part 6: Enable FAIL_ON_WARNINGS in dom/bindings

> Do you actually opt into --enable-warnings-as-errors?

Yes, because otherwise I've ended up checking in patches that break the tree...

> And do those warnings become fatal?

I just tested.  Apparently they do not. Good.

> I think that if your configuration produces errors on code that's green on
> tinderbox, --enable-warnings-as-errors is a bad idea for you :-).

But conversely, if my configuration doesn't have that flag then it builds just fine code that then goes red on tinderbox.  Such is the catch-22 of --enable-warnings-as-errors!

r=me
Attachment #707507 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #25)
> But conversely, if my configuration doesn't have that flag then it builds
> just fine code that then goes red on tinderbox.  Such is the catch-22 of
> --enable-warnings-as-errors!

True, but tinderbox can always go red on code that compiles locally, just because compilers differ across platforms. So I don't use --enable-warnings-as-errors and deal with the increased incidence of an existing failure mode :-).

Thanks!
> True, but tinderbox can always go red on code that compiles locally

Well, true, but I got burned a bunch of times by the warning thing, while bustage for other reasons has been pretty rare for me.  ;)
So a few notes:

1)  I was wrong in comment 25, because I had the flag mis-spelled locally for some reason.  With that fixed these warnings do in fact cause my build to go red, and so does all sorts of necko code, etc.  I can only assume tinderbox doesn't use clang + ccache...

2)  The fact that I didn't have fatal warnings in fact made the perfectly reasonable code in bug 836050 compile in my tree but not in tinderbox.

I hate warnings-as-errors.  :(
> I can only assume tinderbox doesn't use clang + ccache

Or perhaps uses the slower CCACHE_CPP2=1 thing...
Assignee: roc → nobody
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.