604276 - Silence some MSVC warnings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

These appear many times:

d:\dev\tracemonkey\js\src\jscompartment.h(66) : warning C4251: 'JSCompartment::crossCompartmentWrappers' : class 'js::HashMap<Key,Value,HashPolicy,AllocPolicy>' needs to have dll-interface to be used by clients of struct 'JSCompartment'
d:\dev\tracemonkey\js\src\jsscope.h(211) : warning C4307: '+' : integral constant overflow

These appear once:

d:/dev/tracemonkey/js/src/jscntxt.cpp(2049) : warning C4355: 'this' : used in base member initializer list
d:/dev/tracemonkey/js/src/jstracer.cpp(10274) : warning C4099: 'js::BoxArg' : type name first seen using 'class' now seen using 'struct'

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Luke Wagner [:luke]]

Instead of the noisy pragma, you could do:

 struct JSContext {
    JSContext *thisInInitializer() { return this; }
    JSContext() : busyArrays(thisInInitializer()) {}
    ...
 };

Comment 3

[David Mandelin [:dmandelin]]

The |thisInitializer| solution seems better to me too.

On that constant overflow warning, what is the static assertion trying to do, anyway? Can (0xffffffff + 1) be anything other than 0 as a 32-bit value, ever? Maybe we should just delete the assertion.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

Comment 5

[Jason Orendorff [:jorendorff]]

> On that constant overflow warning, what is the static assertion trying to do,
> anyway? Can (0xffffffff + 1) be anything other than 0 as a 32-bit value, ever?
> Maybe we should just delete the assertion.

The assertion really is checking that when we add 1 to SHAPE_INVALID_SLOT, it overflows and produces 0. See the comment on JSShape::setParent, also in jsscope.h.
http://hg.mozilla.org/tracemonkey/diff/b1facf8ba54e/js/src/jsscope.h

Maybe we should just move the static assertion into the body of setParent.

But getting to zero warnings by deleting static assertions would be running the wrong way, I think.

Comment 6

[David Mandelin [:dmandelin]]

(In reply to comment #5)
> > On that constant overflow warning, what is the static assertion trying to do,
> > anyway? Can (0xffffffff + 1) be anything other than 0 as a 32-bit value, ever?
> > Maybe we should just delete the assertion.
> 
> The assertion really is checking that when we add 1 to SHAPE_INVALID_SLOT, it
> overflows and produces 0. See the comment on JSShape::setParent, also in
> jsscope.h.
> http://hg.mozilla.org/tracemonkey/diff/b1facf8ba54e/js/src/jsscope.h
> 
> Maybe we should just move the static assertion into the body of setParent.
> 
> But getting to zero warnings by deleting static assertions would be running the
> wrong way, I think.

Sure, I wouldn't recommend just deleting assertions in general. This one is just hard for me to distinguish from "const int foo = 3; JS_STATIC_ASSERT(foo + 1 == 4);". I would probably just go with a comment like "This must be 0xfffffff so that adding overflows, blah", so that anyone modifying that code would know not to change it lightly. I guess the static assert does add a level of "paranoid" safety in that if someone didn't read the comment and changed the value, they would trip the assert. Moving it to setParent does seem like a good idea to me, maybe with an brief explanation (or pointer to one) as to why this property is required right next to the assert.

Anyway, I don't feel that strongly about this--it was just what popped into my head on reading the v1 patch.

Comment 7

[Mike Shaver (:shaver emeritus)]

re overflow, see Bug 598055

Comment 8

[Jason Orendorff [:jorendorff]]

I landed this before seeing comment 6.
  http://hg.mozilla.org/tracemonkey/rev/14fb745bcbd8

Quick follow-up:
  http://hg.mozilla.org/tracemonkey/rev/9bda4099205e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

This is FIXED, no?

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)

> I landed this before seeing comment 6.
>   http://hg.mozilla.org/tracemonkey/rev/14fb745bcbd8

http://hg.mozilla.org/mozilla-central/rev/14fb745bcbd8

> Quick follow-up:
>   http://hg.mozilla.org/tracemonkey/rev/9bda4099205e

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