Closed
Bug 604276
Opened 14 years ago
Closed 14 years ago
Silence some MSVC warnings
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
(Whiteboard: [fixed-in-tracemonkey])
Attachments
(1 file, 1 obsolete file)
4.17 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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'
Assignee | ||
Comment 1•14 years ago
|
||
Assignee: general → jorendorff
Attachment #483064 -
Flags: review?(dmandelin)
Comment 2•14 years ago
|
||
Instead of the noisy pragma, you could do: struct JSContext { JSContext *thisInInitializer() { return this; } JSContext() : busyArrays(thisInInitializer()) {} ... };
Comment 3•14 years ago
|
||
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.
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #483064 -
Attachment is obsolete: true
Attachment #483229 -
Flags: review?(lw)
Attachment #483064 -
Flags: review?(dmandelin)
Updated•14 years ago
|
Attachment #483229 -
Flags: review?(lw) → review+
Assignee | ||
Comment 5•14 years ago
|
||
> 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•14 years ago
|
||
(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.
re overflow, see Bug 598055
Assignee | ||
Comment 8•14 years ago
|
||
I landed this before seeing comment 6. http://hg.mozilla.org/tracemonkey/rev/14fb745bcbd8 Quick follow-up: http://hg.mozilla.org/tracemonkey/rev/9bda4099205e
Whiteboard: [fixed-in-tracemonkey]
Comment 9•14 years ago
|
||
This is FIXED, no?
Assignee | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 10•14 years ago
|
||
(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
Target Milestone: --- → mozilla2.0b8
Version: Other Branch → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•