Closed Bug 1024318 Opened 6 years ago Closed 6 years ago

Fix warnings in mfbt/tests/ and mark as FAIL_ON_WARNINGS

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

* Fixed clang/gcc warnings:

mfbt/tests/TestBinarySearch.cpp:29:10 [-Wunused-variable] unused variable 'm'
mfbt/tests/TestPoisonArea.cpp:246:0: error: "PAGESIZE" redefined

* Fixed MSVC warnings:

mfbt/tests/TestEndian.cpp(338) : warning C4309: 'argument' : truncation of constant value
mfbt/tests/TestEndian.cpp(340) : warning C4309: 'argument' : truncation of constant value

* Suppressed MSVC warnings:

obj-firefox\dist\include\mozilla/TypeTraits.h(438) : warning C4197: 'volatile bool' : top-level volatile in cast is ignored
c:\PROGRA~2\MICROS~2.0\vc\include\xlocale(323) : warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc
c:\PROGRA~2\MICROS~2.0\vc\include\typeinfo(157) : warning C4275: non dll-interface class 'stdext::exception' used as base for dll-interface class 'std::bad_cast'
Attachment #8438951 - Flags: review?(jwalden+bmo)
Comment on attachment 8438951 [details] [diff] [review]
fix-mfbt-test-warnings.patch

Review of attachment 8438951 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/tests/TestPoisonArea.cpp
@@ +246,1 @@
>  #define PAGESIZE _pagesize

Instead of this, which presumably is defined in some sort of evil system header, could you add

static inline unsigned long
PageSize()
{
  return unixPageSize;
}

and

static inline uint32_t
PageSize()
{
  return sInfo_.dwAllocationGranularity;
}

for the Windows case?  (Please rename away from global variables with names starting with underscores while you're here -- those are reserved in C++.)

::: mfbt/tests/moz.build
@@ +37,5 @@
>  DISABLE_STL_WRAPPING = True
> +
> +if CONFIG['_MSC_VER']:
> +  CXXFLAGS += [
> +    '-wd4197', # top-level volatile in cast is ignored

We can just fix this, I think.  See forthcoming attachment.  Bit of a strange warning for MSVC to emit, tho.  I wonder what their motivation was for adding it.
Attachment #8438951 - Flags: review?(jwalden+bmo) → review+
This probably fixes the Windows issue.  Builds with clang, didn't bother testing any other compilers, seems fairly basic template-fu with little expectation of issues popping up.
Attachment #8439301 - Flags: feedback?(cpeterson)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> We can just fix this, I think.  See forthcoming attachment.  Bit of a
> strange warning for MSVC to emit, tho.  I wonder what their motivation was
> for adding it.

MSDN says: "The compiler detected a cast to an r-value type which is qualified with volatile, or a cast of an r-value type to some type that is qualified with volatile. According to the C standard (6.5.3), properties associated with qualified types are meaningful only for l-value expressions."

http://msdn.microsoft.com/en-us/library/ydet06y0.aspx
Well, sure.  The question is why someone was agitated enough about a non-meaningful cv-qualifier to go to the trouble of adding a warning for it, adding tests for the warning working correctly, doing QA on it, etc.
Comment on attachment 8439301 [details] [diff] [review]
Avoid a useless cast to volatile bool

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Created attachment 8439301 [details] [diff] [review]
> Avoid a useless cast to volatile bool
> 
> This probably fixes the Windows issue.  Builds with clang, didn't bother
> testing any other compilers, seems fairly basic template-fu with little
> expectation of issues popping up.

Unfortunately, your patch did not fix the Windows issue:

https://tbpl.mozilla.org/php/getParsedLog.php?id=41645760&tree=Try#error4
https://tbpl.mozilla.org/?tree=Try&rev=ab37390d44f7


c:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\mozilla/TypeTraits.h(423) : see reference to class template instantiation 'mozilla::detail::IsSignedHelper<T>' being compiled
        with
        [
            T=volatile bool
        ]

c:/builds/moz2_slave/try-w32-0000000000000000000000/build/mfbt/tests/TestTypeTraits.cpp(111) : see reference to class template instantiation 'mozilla::IsSigned<T>' being compiled
        with
        [
            T=volatile bool
        ]

c:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\mozilla/TypeTraits.h(405) : warning C4197: 'volatile bool' : top-level volatile in cast is ignored

c:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\mozilla/TypeTraits.h(405) : warning C4197: 'volatile bool' : top-level volatile in cast is ignored
Attachment #8439301 - Flags: feedback?(cpeterson) → feedback-
(In reply to Chris Peterson (:cpeterson) from comment #0)
> * Suppressed MSVC warnings:
> 
> obj-firefox\dist\include\mozilla/TypeTraits.h(438) : warning C4197:
> 'volatile bool' : top-level volatile in cast is ignored
> c:\PROGRA~2\MICROS~2.0\vc\include\xlocale(323) : warning C4530: C++
> exception handler used, but unwind semantics are not enabled. Specify /EHsc
> c:\PROGRA~2\MICROS~2.0\vc\include\typeinfo(157) : warning C4275: non
> dll-interface class 'stdext::exception' used as base for dll-interface class
> 'std::bad_cast'

Err, the right way to fix this is to define _HAS_EXCEPTIONS=0 as I'm doing for js/src in bug 1024833.
Duh.  Obviously both IsSigned and IsUnsigned need the fix.  At a skim of the rest of TypeTraits.h, this looks complete.
Attachment #8439301 - Attachment is obsolete: true
Attachment #8439678 - Flags: feedback?(cpeterson)
Comment on attachment 8439678 [details] [diff] [review]
More complete attempt

Thanks. This patch fixes the volatile bool warnings.

Ehsan's suggestion (in comment 6) to use _HAS_EXCEPTIONS=0 did not work. On IRC, he begrudgingly agreed we could just suppress MSVC's dll-interface warnings. He suspects this is a problem, possibly our fault, with mismatch dllimport/dllexport macros.
Attachment #8439678 - Flags: feedback?(cpeterson) → feedback+
dllimport/dllexport is tricky to get right, especially for headers that can be used internally and externally. It gets worse for imported third party headers.
Landed r+'d patch plus Waldo's volatile cast and PageSize() changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/315b89ffbe12
https://hg.mozilla.org/mozilla-central/rev/315b89ffbe12
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.