Closed Bug 610480 Opened 9 years ago Closed 9 years ago

Silence current MSVC warnings

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dmandelin, Assigned: dmandelin)

References

Details

Attachments

(3 files)

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?
Requesting review because I don't want to introduce subtle breakage commuting unary minus with cast to signed.
Assignee: general → dmandelin
Status: NEW → ASSIGNED
Attachment #488984 - Flags: review?(jwalden+bmo)
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.
Attachment #489386 - Flags: review?(jorendorff)
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.
Attachment #488984 - Flags: review?(jwalden+bmo) → review+
(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
Whiteboard: fixed-in-tracemonkey
Attachment #489386 - Flags: review?(jorendorff) → review+
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
What about the MarkContext inconsistent dll linkage warning that spams all over the tree? (Bug 554350)
I backed out the third round of warning fixes noted in comment 6, because the new assertions were failing consistently in debug builds.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/9af757c2776a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #492800 - Flags: review?(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.
Attachment #492800 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/tracemonkey/rev/e0318d0c2282

Thanks for the catch! We seem to be free of warnings (except that linker thing) on VS2010.
Duplicate of this bug: 612777
You need to log in before you can comment on or make changes to this bug.