Last Comment Bug 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)
: warnings-as-errors - MSVC C4265: 'js::WeakMap<Key,Value,HashPolicy,MarkPolicy...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows Server 2003
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-13 20:43 PDT by Phil Ringnalda (:philor, back in August)
Modified: 2011-10-21 00:56 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (767 bytes, patch)
2011-09-06 02:02 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review

Description Phil Ringnalda (:philor, back in August) 2011-08-13 20:43:15 PDT
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] 2011-09-06 02:02:39 PDT
Created attachment 558423 [details] [diff] [review]
fix
Comment 2 Luke Wagner [:luke] 2011-09-06 08:29:47 PDT
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, back in August) 2011-10-08 18:25:03 PDT
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] (remove +bmo to email) 2011-10-08 18:48:08 PDT
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] (remove +bmo to email) 2011-10-08 18:51:00 PDT
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).

Note You need to log in before you can comment on or make changes to this bug.