984256 - device_info_linux.cc:166:93: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare]

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning:
{
Warning: -Wsign-compare in /scratch/work/builds/mozilla-inbound/mozilla/media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int')

/scratch/work/builds/mozilla-inbound/mozilla/media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:166:93: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare]
}

The code in question is:
> 166         if (snprintf(deviceUniqueIdUTF8, deviceUniqueIdUTF8Length, "fake_%u", device_index) >=
> 167             deviceUniqueIdUTF8Length)
> 169             WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceVideoCapture, _id,
> 170                        "buffer passed is too small");
> 171             return -1;
...where deviceUniqueIdUTF8Length is a uint32_t, and snprintf returns an int.

According to [1], "If an encoding error occurs, a negative number is returned." (by snprintf)

It looks like we'll probably do the right thing (bail) in cases where that happens, but it'd probably be better to more explicitly handle the "< 0" case (possibly with a different warning, since "buffer passed is too small" isn't really correct in that case)

[1] http://www.cplusplus.com/reference/cstdio/snprintf/

Perhaps this should be filed upstream somewhere?

Comment 1

[Daniel Holbert [:dholbert]]

(This was added here:
http://hg.mozilla.org/mozilla-central/diff/bd8f1571937f/media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc#l1.35
in bug 932112)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Note that _snprintf is often substituted for snprintf, but it doesn't have the same return value semantics (or guarantee of null-termination if the buffer is too small!)

That said, this code avoids snprintf generally - and this doesn't come from 3.43, it's part of our changes.  I'd also be totally good with adding "or error" to the trace message.  This is just for debugging, so there's no need to even change the error IMHO.

Comment 3

[Daniel Holbert [:dholbert]]

This was fixed by adding an "(int)" cast, in a changeset for bug 1237023:
>         if (snprintf(deviceUniqueIdUTF8, deviceUniqueIdUTF8Length, "fake_%u", device_index) >=
>-            deviceUniqueIdUTF8Length)
>+            (int) deviceUniqueIdUTF8Length)
https://hg.mozilla.org/mozilla-central/rev/4bd447a3d7da#l11.10

Resolving as FIXED by that bug.