Closed
Bug 1273079
Opened 8 years ago
Closed 8 years ago
Use MOZ_MUST_USE in hal
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Core
Hardware Abstraction Layer (HAL)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: wcpan, Assigned: wcpan)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
It would be better to check return values for some functions, e.g. LockScreenOrientation().
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → wpan
Blocks: use-nodiscard
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52762/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52762/
Attachment #8752743 -
Flags: review?(gsvelto)
Comment 2•8 years ago
|
||
Comment on attachment 8752743 [details] MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto https://reviewboard.mozilla.org/r/52762/#review49962 Looks good to me with only one small nit. ::: dom/base/ScreenOrientation.cpp:557 (Diff revision 1) > { > if (aOrientation == eScreenOrientation_None) { > hal::UnlockScreenOrientation(); > } else { > - hal::LockScreenOrientation(aOrientation); > + bool rv = hal::LockScreenOrientation(aOrientation); > + NS_WARN_IF(!rv); nit: You can use `NS_WARN_IF_FALSE(rv)` here
Attachment #8752743 -
Flags: review?(gsvelto) → review+
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8752743 [details] MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52762/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 4•8 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #2) > Comment on attachment 8752743 [details] > MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto > > https://reviewboard.mozilla.org/r/52762/#review49962 > > Looks good to me with only one small nit. > > ::: dom/base/ScreenOrientation.cpp:557 > (Diff revision 1) > > { > > if (aOrientation == eScreenOrientation_None) { > > hal::UnlockScreenOrientation(); > > } else { > > - hal::LockScreenOrientation(aOrientation); > > + bool rv = hal::LockScreenOrientation(aOrientation); > > + NS_WARN_IF(!rv); > > nit: You can use `NS_WARN_IF_FALSE(rv)` here Just a note that NS_WARN_IF_FALSE is noop in release build. Although it doesn't change the semantic here, beware the difference when you want to use it next time. See also bug 1272010
Comment 5•8 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #4) > Just a note that NS_WARN_IF_FALSE is noop in release build. Although it > doesn't change the semantic here, beware the difference when you want to use > it next time. > > See also bug 1272010 Thanks for the heads up. In this case I think this behavior is fine as this is an error very unlikely to occur.
Comment 7•8 years ago
|
||
Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/7b2809db7c76 https://treeherder.mozilla.org/logviewer.html#?job_id=28087764&repo=mozilla-inbound#L8698
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8752743 [details] MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52762/diff/2-3/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/52762/#review49990 Silly me for not spotting the issue in the previous review :-/
Assignee | ||
Comment 10•8 years ago
|
||
I should compile it first anyway. I thought NS_WARN_IF_FALSE was just a negative version of NS_WARN_IF.
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a906d38b7c07
Keywords: checkin-needed
Comment 12•8 years ago
|
||
Backed out for bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/89c6cb3b0860 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=11db59507360d0d72aa72301d064c8fd2b0e926e Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=28237315&repo=mozilla-inbound dom/base/ScreenOrientation.cpp:556:10: error: unused variable 'rv' [-Werror=unused-variable]
Flags: needinfo?(wpan)
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8752743 [details] MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52762/diff/3-4/
Assignee | ||
Comment 14•8 years ago
|
||
Ouch, I didn't know that release build will turn on warning-as-error. :gsvelto, I guess maybe I need to use the first patch. At least it passed try-build. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e175325ee2f4 https://treeherder.mozilla.org/#/jobs?repo=try&revision=950ad9b7ddf0
Flags: needinfo?(wpan) → needinfo?(gsvelto)
Comment 15•8 years ago
|
||
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #14) > :gsvelto, I guess maybe I need to use the first patch. At least it passed > try-build. Yes please, always run a try build on all architectures in both debug and release mode before landing. You never know what might break.
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8752743 [details] MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52762/diff/4-5/
Assignee | ||
Comment 17•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=217be6bd02a67ba698199424b135debcfd8a50fa
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfd273902ac
Keywords: checkin-needed
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fbfd273902ac
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•