Bug 30631 – Suppress unavoidable MSVC warnings during wxWebKit build

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit wx

[Vadim Zeitlin]

Created attachment 41564 [details]
Patch to disable several more MSVC warnings

Currently building wxWebKit results in tons of output due to unimportant or unavoidable MSVC warnings which prevent you from seeing any important ones. Some of them were already disabled but I'd like to add a few more. Quoting the attached patch:

Disable several warnings which occur many times during the build.
Some of them are harmless (4099, 4344, 4396, 4800) and working around
them in WebKit code is probably just not worth it. We can simply do
nothing about the others (4503). A couple are possibly valid but
there are just too many of them in the code so fixing them is
impossible in practice and just results in tons of distracting output
(4244, 4291). Finally 4996 is actively harmful as it is given for
just about any use of standard C/C++ library facilities.


TIA!

[Eric Seidel (no email)]

Comment on attachment 41564 [details]
Patch to disable several more MSVC warnings

I'm glad to see these documented.  We're slowly getting rid of the struct/class warnings.

Are you sure the rest of these should not be turned into bugs?  I think some of these should be made into bugs and the bug numbers added as comments next to those ignore lines.

I'm very very very glad that you're documenting the type of warning next to the code.  That's much better than how the file looked previously.

Please strongly consider turning some of these into WebKit bugs and adding the bug numbers to the settting.py comments as well.

If you want this committed by the commit-bot you'll need to set commit-queue=?

[Vadim Zeitlin]

(In reply to comment #1)
> (From update of attachment 41564 [details])

Thanks for your quick review!

> I'm glad to see these documented.  We're slowly getting rid of the struct/class
> warnings.

I had a patch correcting 3-4 of them and I probably could still find it (I dropped the branch but it must be in reflog yet) if you think this could be useful. FWIW it's probably not the most useful warning in the world, I can't imagine it indicating any _real_ problems.
 
> Are you sure the rest of these should not be turned into bugs?

I think the only one which could be worth fixing is C4244: "conversion from 'xxx' to 'yyy', possible loss of data". Unfortunately there are probably thousands of occurrences of this warning in WebKit (just as in any non-trivial C++ program because of implicit conversions inherited from C) so I don't know if it's realistic to hope to ever fix it.

I also reviewed a couple of occurrences of C4291 and I think it's safe to ignore it in this case. I didn't check all of them though.

The rest are either useless or can't be fixed anyhow.

> Please strongly consider turning some of these into WebKit bugs and adding the bug numbers to the settting.py comments as well.

Please let me know if you'd [still] like me to open bugs for 4244 and/or 4291.
Also, should I open bugs for other, more serious (IMO), warnings which this patch does not suppress?

> If you want this committed by the commit-bot you'll need to set commit-queue=?

Thanks, I'll do it after updating the patch to mention the new bug numbers (if any). BTW, is there any convention for referring to the bugs in WebKit sources, e.g. "#NNNN" or "bug NNNN"? Sorry if this is answered elsewhere but I didn't find it in http://webkit.org/coding/contributing.html and couldn't find any references to the existing bugs in the sources neither.

TIA!

[Eric Seidel (no email)]

(In reply to comment #2)
> Please let me know if you'd [still] like me to open bugs for 4244 and/or 4291.
> Also, should I open bugs for other, more serious (IMO), warnings which this
> patch does not suppress?

You should open bugs for any issues you feel are serious.  Your discretion.

> > If you want this committed by the commit-bot you'll need to set commit-queue=?
> 
> Thanks, I'll do it after updating the patch to mention the new bug numbers (if
> any). BTW, is there any convention for referring to the bugs in WebKit sources,
> e.g. "#NNNN" or "bug NNNN"? Sorry if this is answered elsewhere but I didn't
> find it in http://webkit.org/coding/contributing.html and couldn't find any
> references to the existing bugs in the sources neither.

We normally just use the full url in source code.  Like:
https://bugs.webkit.org/show_bug.cgi?id=30631

If you want a patch cq+'d it will need to be r+'d as well, so if you post a new patch make sure to mark it both r? and cq?

[Eric Seidel (no email)]

Comment on attachment 41564 [details]
Patch to disable several more MSVC warnings

Landing this.  Vadim can provide additional patches to add bug urls if/when he files them.

[WebKit Commit Bot]

Comment on attachment 41564 [details]
Patch to disable several more MSVC warnings

Clearing flags on attachment: 41564

Committed r50135: <http://trac.webkit.org/changeset/50135>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.