1114444 - Fix clang warnings in js/src/gc/Statistics.cpp

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: fix-Statistics-warnings.patch

These are new compiler warnings in the diagnostics code added for bug 1103957:

js/src/gc/Statistics.cpp:889:24: warning: format specifies type 'long' but the argument has type 'int64_t' (aka 'long long') [-Wformat]
                phase, phaseStartTimes[phase]);
                       ^~~~~~~~~~~~~~~~~~~~~~

js/src/gc/Statistics.cpp:890:27: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
        for (int i = 0; i < phaseNestingDepth; i++)
                        ~ ^ ~~~~~~~~~~~~~~~~~

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 8539921 [details] [diff] [review]
fix-Statistics-warnings.patch

If the argument has a <stdint.h> type, the right thing to use is the macros in #include "mozilla/IntegerPrintfMacros.h".  "%lld" is not guaranteed to match int64_t.

%zu isn't supported by MSVC.  I see someone sub silentio added PRIuSIZE to IntegerPrintfMacros.h for this.  But this violates the intent of the header: that its supported interface be *only* a subset of <inttypes.h>, with the goal to swap that in when bug-free <inttypes.h> is provided by all supported platforms.  So don't use that, either.  (I guess I need to file a bug to remove it.)  For now just cast the fprintf argument to uintptr_t and use PRIuPTR til we figure out something to provide an alternative to %z.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: fix-Statistics-warnings-v2.patch

Patch v2: fprintf uint64_t with PRId64 and size_t with PRIuPTR.

Green Try build:
https://tbpl.mozilla.org/?tree=Try&rev=3f6f46f0f0ae

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8540306 [details] [diff] [review]
fix-Statistics-warnings-v2.patch

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

Gah. More to back out when these diagnostics bear fruit. Sorry about the warnings.

Comment 4

[Steve Fink [:sfink] [:s:]]

Note that I have a fix for the actual problem now, so the code containing the warnings will be removed once that's r+. So  It's up to you whether you want to push this in the meantime.

Comment 5

[Steve Fink [:sfink] [:s:]]

I pushed the other fix, so this code is gone.