Closed
Bug 1153730
Opened 9 years ago
Closed 9 years ago
libstagefright/MPEG4Extractor.cpp:2059:29: warning: comparison of constant '2' with boolean expression is always false [-Wbool-compare]
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 1219475
Tracking | Status | |
---|---|---|
firefox40 | --- | affected |
People
(Reporter: dholbert, Unassigned)
References
(Blocks 1 open bug)
Details
Build warning, with clang 3.6 (probably other compilers as well): { media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:2059:29: warning: comparison of constant '2' with boolean expression is always false [-Wbool-compare] if (!sap || saptype > 2) { ^ } This is in code that we imported in bug 908503. Code snippet, before the warning: > 2046 uint32_t d1, d2, d3; > 2047 > 2048 if (!mDataSource->getUInt32(offset, &d1) || // size > 2049 !mDataSource->getUInt32(offset + 4, &d2) || // duration > 2050 !mDataSource->getUInt32(offset + 8, &d3)) { // flags > 2051 return ERROR_MALFORMED; > 2052 } [...] > 2058 bool saptype = d3 >> 28; > 2059 if (!sap || saptype > 2) { > 2060 ALOGW("not a stream access point, or unsupported type"); > 2061 } Basically: in the "saptype > 2" conversion, saptype is promoted to a uint32_t. A bool --> uint32_t promotion can only produce the values 0 or 1. So, even if saptype's source-data was something greater than 2, that information is lost as soon as it's crammed into a bool. (and then promoted back out of the bool later on for the comparison) I'm guessing it's not that important of a check, since the only result is some logging. Still, there's no point in performing a false-100%-of-the-time-by-definition comparison. Someone (maybe me) should probably file this upstream. Filing it on our end for now, to have a record of it here, since we have this code in our tree.
Reporter | ||
Comment 1•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #0) > Basically: in the "saptype > 2" conversion, saptype is promoted to a uint32_t (er, promoted to an int, I think, to match the type of the literal '2'. Anyway, the point remains.)
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: Playback
From the specs this SAP-type field is a 3-bit value at bits 28-30 in 'd3', so we should probably shift&mask it into an unsigned integer type. SAP types 1 and 2 are known (and a 0-value is valid as well here), so the test |saptype > 2| is correct, assuming we fix the type. The only problem would be that a file with a SAP type of 3 would not show the warning. I'll investigate a bit further to ensure my assumptions are correct, before proposing a patch.
Assignee: nobody → gsquelart
Getting fixed in bug 1219475.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Updated•9 years ago
|
Assignee: gsquelart → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•