678783 - warnings-as-errors - MSVC C4265: 'js::WeakMap<Key,Value,HashPolicy,MarkPolicy>' : class has virtual functions, but destructor is not virtual in jsweakmap.h(237)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1313260422.1313260737.561.gz&fulltext=1#err0
mozilla-inbound_win32_spidermonkey-warnaserr on 2011/08/13 11:33:42 

jscompartment.cpp

e:\builds\moz2_slave\m-in-w32-spidermonkey-warnaserr\src\js\src\jsweakmap.h(237) : error C2220: warning treated as error - no 'object' file generated

        e:\builds\moz2_slave\m-in-w32-spidermonkey-warnaserr\src\js\src\vm/Debugger.h(112) : see reference to class template instantiation 'js::WeakMap<Key,Value,HashPolicy,MarkPolicy>' being compiled

        with

        [

            Key=JSObject *,

            Value=JSObject *,

            HashPolicy=js::DefaultHasher<JSObject *>,

            MarkPolicy=js::CrossCompartmentMarkPolicy

        ]

e:\builds\moz2_slave\m-in-w32-spidermonkey-warnaserr\src\js\src\jsweakmap.h(237) : warning C4265: 'js::WeakMap<Key,Value,HashPolicy,MarkPolicy>' : class has virtual functions, but destructor is not virtual

         instances of this class may not be destructed correctly

        with

        [

            Key=JSObject *,

            Value=JSObject *,

            HashPolicy=js::DefaultHasher<JSObject *>,

            MarkPolicy=js::CrossCompartmentMarkPolicy

        ]

e:\builds\moz2_slave\m-in-w32-spidermonkey-warnaserr\src\js\src\vm/Debugger.h(405) : warning C4099: 'JSCompartment' : type name first seen using 'struct' now seen using 'class'

        e:\builds\moz2_slave\m-in-w32-spidermonkey-warnaserr\src\js\src\jscompartment.h(391) : see declaration of 'JSCompartment'

e:\builds\moz2_slave\m-in-w32-spidermonkey-warnaserr\src\js\src\vm/Debugger.h(443) : warning C4099: 'JSCompartment' : type name first seen using 'struct' now seen using 'class'

        e:\builds\moz2_slave\m-in-w32-spidermonkey-warnaserr\src\js\src\jscompartment.h(391) : see declaration of 'JSCompartment'

Comment 1

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 558423 [details] [diff] [review]
fix

Maybe it doesn't matter, but I think the fix is to use a #pragma to silence the warning; this looks intentional.

Comment 3

[Phil Ringnalda (:philor)]

Because he doesn't believe bugs for warnings exist, Waldo landed that patch in http://hg.mozilla.org/mozilla-central/rev/ec7d58602263, dunno whether anyone wants to keep this open to consider whether or not doing something else is worthwhile.

Comment 4

[Jeff Walden [:Waldo]]

I hadn't realized this bug was filed, mea culpa.

Actually, I mis-tagged that commit.  I discussed the problem with Luke at the time, and I did go down the #pragma road, briefly.  But you hit a problem, that you have to #pragma away every base class if you're going to try it.  We also tried making ~WeakMapBase protected and seeing if that would make MSVC happy, but it didn't either.  And Luke also expressed, after I grumbled about this a bit, that just making it virtual couldn't really matter, for the reasons I noted in that commit message.  So in the end I just did the simple thing and made it virtual.

As for not believing in bugs for warnings, it depends on the complexity of the fix.  (Bug 693100 is a counter-example, filed before comment 3, and before I noticed this bug.)  In this case I wanted to go from burning all the way to green, and filing bugs for so many really minor things just felt like stop-energy.

Comment 5

[Jeff Walden [:Waldo]]

I'm going to say it's not really worthwhile doing something else here.  If people disagree, they can reopen.  I have no particular love for the fix I made, over other possibilities (except to the extent that it's simpler than splashing #pragmas a few places).