Closed Bug 578546 Opened 9 years ago Closed 9 years ago
warning C4290: C++ exception specification ignored except to indicate a function is not __declspec(nothrow)
Another big source of warning spam when building with MSVC10. In dist\include\mozilla\mozalloc.h(224)
This also happens on MSVC8.
Chris, any chance this can get any love? It makes it very hard to look for other warnings because this and bug 578540 are so overwhelmingly spammed to the console.
Sorry, was on PTO. I won't be able to look at this until probably next week. If someone else wants to take a crack, the patch will likely reduce to making the |throw()| specifier disappear on MSVC.
According to MSDN, we can otherwise kill this with #pragma warning( disable : 4290 ) http://msdn.microsoft.com/en-us/library/sa28fef8%28VS.80%29.aspx Otherwise, do I understand your last comment to mean changing the conditions on the ifdef on line #207 of mozalloc.h? http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#207
(In reply to comment #4) > Otherwise, do I understand your last comment to mean changing the conditions on > the ifdef on line #207 of mozalloc.h? > http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#207 Sure, sounds good.
To make sure I understand what's going on here, right now we're defining MOZALLOC_THROW_IF_HAS_EXCEPTIONS as throw() for all compilers other than Android. Since we aren't building with exceptions enabled on MSVC, the __declspec(nothrow) is being automatically used to disable that throw() along with warning about having done so. Therefore, we should just change the ifdef on line #207 to include both Android and MSVC to make this warning go away, and doing so shouldn't have any other effect since the throw() is effectively being disabled anyway right now.
Starting at line 207, I made the following change: -#ifdef ANDROID +#if defined(ANDROID) || defined(_MSC_VER) /* * Android doesn't fully support exceptions, so its <new> header - * has operators that don't specify throw() at all. + * has operators that don't specify throw() at all. Also include MSVC + * to suppress build warning spam (bug 578546). */ Seems to have worked, except I'm realizing that we're also getting the same warning on lines 225 and 237 (on the MOZALLOC_THROW_BAD_ALLOC lines). However, I can't say with certainty that those are new and were just missed before due to the prevalence of the other warnings. Starting on line 217, we have: #ifdef MOZ_CPP_EXCEPTIONS #define MOZALLOC_THROW_BAD_ALLOC throw(std::bad_alloc) #else #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_IF_HAS_EXCEPTIONS #endif So my assumption is the MOZ_CPP_EXCEPTIONS is set for MSVC and is hence giving us the throw(std::bad_alloc) line. Not sure how to proceed on this one since I'm assuming MOZ_CPP_EXCEPTIONS is defined on purpose here.
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Attachment #487798 - Flags: review?(jones.chris.g)
(In reply to comment #6) > To make sure I understand what's going on here, right now we're defining > MOZALLOC_THROW_IF_HAS_EXCEPTIONS as throw() for all compilers other than > Android. Since we aren't building with exceptions enabled on MSVC, the > __declspec(nothrow) is being automatically used to disable that throw() along > with warning about having done so. Not quite ... the MSDN page explains A function is declared using exception specification, which Visual C++ accepts but does not implement. Code with exception specifications that are ignored during compilation may need to be recompiled and linked to be reused in future versions supporting exception specifications. VC parses |throw([X])| syntax but ignores it thereafter. Technically these functions *are* __declspec(nothrow) but since we don't build with exceptions it shouldn't matter.
MOZ_CPP_EXCEPTIONS shouldn't be set unless you build with --enable-cpp-exceptions, or are in code where it's forced on.
Attachment #487798 - Flags: review?(jones.chris.g) → review+
Looks like MOZ_CPP_EXCEPTIONS is coming in from the IPC code setting ENABLE_CXX_EXCEPTIONS=1 which then adds MOZ_CPP_EXCEPTIONS=1 to CXXFLAGS. Another one of those situations where I don't understand the code well enough to know what we should do here.
Forgot to say, rules.mk is what does the CXXFLAGS setting. http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#375
Hmm, I sure thought we had removed ENABLE_CXX_EXCEPTIONS=1 from the IPC stuff. bent, do you remember why we needed it in the first place? Ryan, I would try building without it. It'd be better to just remove that than work around it.
hmm, ENABLE_CXX_EXCEPTIONS=1 is in more places than just the IPC code. Not sure if it's safe to remove them all... http://mxr.mozilla.org/mozilla-central/search?find=%2F&string=ENABLE_CXX_EXCEPTIONS
The only two I'm not sure about there are in lib7z and widget/src/windows. The others were added for ipc code or testing (or activex, which we don't care about). widget/src/windows is IPC-enabled now (not sure why ...) so I'm pretty sure we ENABLE_CXX_EXCEPTIONS=1 there for that too.
widget/src/windows goes back to the original cvs-->hg merge. I'm going to try removing it and see what happens. However, I don't see anything within widget/src/windows actually using MOZ_CPP_EXCEPTIONS or MOZ_CXX_EXCEPTIONS, so I'm going to go ahead and just remove it as well.
Warning C4290 is completely gone with this patch and the first one applied. Victory!
Comment on attachment 488379 [details] [diff] [review] Remove ENABLE_CXX_EXCEPTIONS from tree Looks awesome, if tryserver is happy :).
Attachment #488379 - Flags: review?(jones.chris.g) → review+
Passes try with only the usual random orange suspects. Who can a2.0+ these patches?
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.