610480 - Silence current MSVC warnings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

1. Most of them are like this:

c:/sources/tracemonkey/js/src/jsscope.cpp(921) : warning C4242: '=' : conversion from 'uintN' to 'uint8', possible loss of data
c:/sources/tracemonkey/js/src/jsscope.cpp(923) : warning C4242: '=' : conversion from 'intN' to 'int16', possible loss of data
c:/sources/tracemonkey/js/src/jsscript.cpp(1040) : warning C4242: '=' : conversion from 'uint32' to 'uint16', possible loss of data
c:/sources/tracemonkey/js/src/jsscript.cpp(1041) : warning C4242: '=' : conversion from 'uint32' to 'uint16', possible loss of data
c:/sources/tracemonkey/js/src/yarr/yarr/RegexCompiler.cpp(97) : warning C4244: 'initializing' : conversion from 'UChar' to 'char', possible loss of data

It is easy enough to silence these by adding casts, but I wonder if that's really a good idea. Should we just turn off this pair of warnings in the makefile?

2. And we've had a couple of these for a long time:

c:/sources/tracemonkey/js/src/jskwgen.cpp(440) : warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
c:/sources/tracemonkey/js/src/jsoplengen.cpp(73) : warning C4996: 'fopen': This
function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.

I assume we are not going to use fopen_s any time in the near future and should just hush these with an option?

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Part 1: Patch for easy warnings

Requesting review because I don't want to introduce subtle breakage commuting unary minus with cast to signed.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Patch 2: Disable some useless warnings

I wanted to disable 4242 this way as well, but it doesn't work. It may relate to the fact that that warning is disabled by default, and presumably enabled by -W3. I tried having the wd before and after the -W3 but it doesn't seem to work either way. I guess I'll just have to add the casts for that later.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 488984 [details] [diff] [review]
Part 1: Patch for easy warnings

I think the signedness bit's fine, based on the current values for JSID_INT_MIN and all.

The opportunity is here: could you add a comment next to their definition asking people who change them to verify that the new values comport with js_CheckForStringIndex?  The last bug in that algorithm was introduced because the range changed but our string-index-checking code didn't.

Comment 4

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> The opportunity is here: could you add a comment next to their definition
> asking people who change them to verify that the new values comport with
> js_CheckForStringIndex? 

Done.

http://hg.mozilla.org/tracemonkey/rev/9af757c2776a

Comment 5

[David Mandelin [:dmandelin]]

Part 2 landed to TM:

http://hg.mozilla.org/tracemonkey/rev/7ad090d53861

Comment 6

[David Mandelin [:dmandelin]]

And a few more warning fixes:

http://hg.mozilla.org/tracemonkey/rev/b6486db91e91

Now it's really done: the only warning left is:

jsinvoke.obj : warning LNK4221: no public symbols found; archive member will be
inaccessible

Comment 7

[Ryan VanderMeulen [:RyanVM]]

What about the MarkContext inconsistent dll linkage warning that spams all over the tree? (Bug 554350)

Comment 8

[Jeff Walden [:Waldo]]

I backed out the third round of warning fixes noted in comment 6, because the new assertions were failing consistently in debug builds.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9af757c2776a

Comment 10

[David Mandelin [:dmandelin]]

Attachment: Patch 3.1: jsscript.cpp warnings

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 492800 [details] [diff] [review]
Patch 3.1: jsscript.cpp warnings

>     CG_COUNT_FINAL_SRCNOTES(cg, nsrcnotes);
>+    uint16 nClosedArgs = cg->closedArgs.length();
>+    JS_ASSERT(nClosedArgs == cg->closedArgs.length());
>+    uint16 nClosedVars = cg->closedVars.length();
>+    JS_ASSERT(nClosedVars == cg->closedVars.length());

This causes warnings in MSVC at least; the assignments need explicit casts.

d:/dev/tracemonkey/js/src/md-objdir/../jsscript.cpp(1163) : warning C4242: 'initializing' : conversion from 'size_t' to 'uint16', possible loss of data
d:/dev/tracemonkey/js/src/md-objdir/../jsscript.cpp(1165) : warning C4242: 'initializing' : conversion from 'size_t' to 'uint16', possible loss of data

r=me with that.

Comment 12

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/e0318d0c2282

Thanks for the catch! We seem to be free of warnings (except that linker thing) on VS2010.