Closed
Bug 1273079
Opened 9 years ago
Closed 9 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•9 years ago
|
Assignee: nobody → wpan
Blocks: use-nodiscard
|
|
Assignee | |
Comment 1•9 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•9 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•9 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•9 years ago
|
Keywords: checkin-needed
|
||
Comment 4•9 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•9 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.
Keywords: checkin-needed
|
||
Comment 7•9 years ago
|
||
|
|
Assignee | |
Comment 8•9 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•9 years ago
|
Keywords: checkin-needed
|
|
||
Comment 9•9 years ago
|
||
https://reviewboard.mozilla.org/r/52762/#review49990
Silly me for not spotting the issue in the previous review :-/
|
|
Assignee | |
Comment 10•9 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•9 years ago
|
||
Keywords: checkin-needed
|
||
Comment 12•9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
|
||
Comment 18•9 years ago
|
||
Keywords: checkin-needed
|
||
Comment 19•9 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 9 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
•