Questionable narrowing conversions (via clang-tidy) in Performance.cpp
Categories
(Core :: Performance: General, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox102 | --- | fixed |
People
(Reporter: mcomella, Assigned: mcomella)
Details
Attachments
(3 files)
### Basic information
Steps to Reproduce:
./mach static-analysis check dom/performance/Performance.cpp
Expected Results:
No warnings
Actual Results:
/Users/mcomella/dev/mozilla-central/dom/performance/Performance.cpp:139:16: warning: narrowing conversion from 'uint64_t' (aka 'unsigned long long') to signed type 'int64_t' (aka 'long long') is implementation-defined [cppcoreguidelines-narrowing-conversions]
rawTime, GetRandomTimelineSeed(), mSystemPrincipal,
^
/Users/mcomella/dev/mozilla-central/dom/performance/Performance.cpp:426:8: warning: narrowing conversion from 'DOMHighResTimeStamp' (aka 'double') to 'bool' [cppcoreguidelines-narrowing-conversions]
if (!ts) {
^
/Users/mcomella/dev/mozilla-central/dom/performance/Performance.cpp:668:17: warning: narrowing conversion from 'uint64_t' (aka 'unsigned long long') to 'double' [cppcoreguidelines-narrowing-conversions]
init.mEpoch = aEpoch;
The first and third warnings seem potentially concerning – we're converting unsigned types into signed types. The second warning doesn't seem as concerning.
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 1•3 years ago
|
||
The converted value gets passed into TimingNotification which assigns the value
into a PerformanceEntry (this is the only usage). Since PerformanceEntry is
defined in WebIDL, we could not change its types (which is double for mEpoch) so
we were forced to safely convert the value where we did.
I don't think the existing conversion code had any bugs since we converted a
64-bit signed integer timestamp into uint64_t (safe) into a double (which is
safe for the reasons mentioned in the code comments).
|
|
Assignee | |
Comment 2•3 years ago
|
||
The implementation of GetRandomTimelineSeed either returns 0 or calls a function
which returns an int64_t. GetRandomTimelineSeed performed an implicit conversion
to uint64_t via its return value. However, all of its callers passed it as the
second argument to nsRFPService::ReduceTimePrecisionAsMSecs (int64_t) or to
RTCStatsTimestampMaker.mRandomTimelineSeed (uint64_t) which is then only used by
ReduceTimePrecisionAsMSecs (int64_t) such that, in practice, only int64_t is
needed. Therefore, we were able to change the return value of
GetRandomTimelineSeed to avoid the narrowing conversion altogether and change
the type of mRandomTimelineSeed to avoid the cascading change.
I don't think the existing conversion code had any bugs since we converted
a int64_t into a uint64_t and back to a int64_t without mutation so we wouldn't
lose any information.
Depends on D145142
|
|
Assignee | |
Comment 3•3 years ago
|
||
This addresses a narrowing conversion warning in clang-tidy. I don't think there
was a bug prior to this - clang-tidy just doesn't seem to like converting
numeric types into an if statement bool even though it should be safe.
Depends on D145143
|
||
Comment 5•3 years ago
|
||
| bugherder | ||
Description
•