Closed Bug 1114444 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix-Statistics-warnings.patch (obsolete) — Splinter Review
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++) ~ ^ ~~~~~~~~~~~~~~~~~
Attachment #8539921 - Flags: review?(sphink)
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.
Attachment #8539921 - Flags: review?(sphink)
Patch v2: fprintf uint64_t with PRId64 and size_t with PRIuPTR. Green Try build: https://tbpl.mozilla.org/?tree=Try&rev=3f6f46f0f0ae
Attachment #8539921 - Attachment is obsolete: true
Attachment #8540306 - Flags: review?(jwalden+bmo)
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.
Attachment #8540306 - Flags: review?(jwalden+bmo) → review+
Attachment #8540306 - Flags: review+
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.
I pushed the other fix, so this code is gone.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: