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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: philor, Unassigned)

Tracking

Trunk
x86
Windows Server 2003
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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'
Created attachment 558423 [details] [diff] [review]
fix

Updated

6 years ago
Attachment #558423 - Flags: review?(luke)

Comment 2

6 years ago
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.
Attachment #558423 - Flags: review?(luke) → review?(anygregor)
(Reporter)

Comment 3

6 years ago
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.
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.
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).
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Attachment #558423 - Flags: review?(anygregor)
You need to log in before you can comment on or make changes to this bug.