Mark widget/android, mozglue/android, and image/decoders/icon/android as FAIL_ON_WARNINGS

RESOLVED FIXED in Firefox 21

Status

()

Core
Widget: Android
P4
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla21
All
Android
Points:
---

Firefox Tracking Flags

(firefox20 wontfix, firefox21 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 706269 [details] [diff] [review]
fix-android-warnings

The warnings included unused variables, signed/unsigned int comparisons, and empty `if` bodies when some debug macros were #undef'd.

Green try run:
https://tbpl.mozilla.org/?tree=Try&rev=488090ced3c4
Attachment #706269 - Flags: review?(bugmail.mozilla)
Comment on attachment 706269 [details] [diff] [review]
fix-android-warnings

Review of attachment 706269 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #706269 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 706269 [details] [diff] [review]
fix-android-warnings

>@@ -1816,17 +1816,19 @@ AndroidBridge::QueueSmsRequest(nsISmsReq
>     // XXX: This method will always fail on Android because we do not
>     // init sSmsRequests. See bug 775997 and Bug 809459.
> 
>     if (!sSmsRequests) {
>         // Probably shutting down.
>         return -1;
>     }
> 
>-    uint32_t length = sSmsRequests->Length();
>+    int32_t length = static_cast<int32_t>(sSmsRequests->Length());
>+    MOZ_ASSERT(length >= 0);
>+
>     for (int32_t i = 0; i < length; i++) {
>         if (!(*sSmsRequests)[i]) {
>             (*sSmsRequests)[i] = aRequest;
>             return i;

While static_cast can silence signed/unsigned issues, IMHO it should be a last resort when really there's nothing else we can reasonably do.

In this case, QueueSmsRequest just needs to return an array-index or a sentinel value.  Seems like it should return the same thing that the array uses -- so, uint32_t, with nsTArray::NoIndex (positive infinity) as its sentinel value.

(That'd require a (trivial) tweak to the callers' sanity-checks, of course)

Then, no casts would be needed (here or on in DequeueSmsRequest).
(Assignee)

Comment 3

5 years ago
Created attachment 706516 [details] [diff] [review]
part-2-fix-AndroidBridge-warnings.patch

Rather than conflating an array index and an error sentinel, I changed the QueueSmsRequest() function to return a bool to indicate success and to return the array index through an out-parameter.

Using a pointer to uint32_t also provides stronger typing because C++ won't coerce pointers to different types like it will signed and unsigned ints.
Attachment #706516 - Flags: review?(dholbert)
Comment on attachment 706516 [details] [diff] [review]
part-2-fix-AndroidBridge-warnings.patch

 >-int32_t
>-AndroidBridge::QueueSmsRequest(nsISmsRequest* aRequest)
>+bool
>+AndroidBridge::QueueSmsRequest(nsISmsRequest* aRequest, uint32_t* aRequestIdOut)
> {
>     NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");> 

Probably worth asserting that aRequestIdOut is non-null.  (Maybe aRequest, too, if it makes sense? looks like it probably does.)

>     // XXX: This method will always fail on Android because we do not
>     // init sSmsRequests. See bug 775997 and Bug 809459.

Looks like this comment is no longer true (I hope), right?  This is code in "AndroidBridge", so I'd hope it works on Android. :)  And the bugs that mentioned there are both fixed. (though I haven't looked at them in detail)

If the comment is indeed incorrect/stale, you might as well delete it as long as you're in the neighborhood.

>-AndroidBridge::DequeueSmsRequest(int32_t aRequestId)
>+AndroidBridge::DequeueSmsRequest(uint32_t aRequestId)
> {
>     NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> 
>-    if (!sSmsRequests || (aRequestId >= sSmsRequests->Length())) {
>+    if (!sSmsRequests || aRequestId >= sSmsRequests->Length()) {
>         return nullptr;

Hopefully we're not *expecting* to receive aRequestIDs beyond our length, right?  Probably worth adding an assertion to check that aRequestId is in-range, so we can detect that sort of issue rather than just silently returning.

Thanks!
Attachment #706516 - Flags: review?(dholbert) → review+
Comment on attachment 706516 [details] [diff] [review]
part-2-fix-AndroidBridge-warnings.patch

> #ifdef DEBUG
> #define ALOG_BRIDGE(args...) ALOG(args)
> #else
>-#define ALOG_BRIDGE(args...)
>+#define ALOG_BRIDGE(args...) ((void)0)
> #endif

Also: it looks like we don't use ALOG_BRIDGE or ALOG at all in this file, so we might as well just drop this chunk rather than tweaking it.
(Assignee)

Comment 6

5 years ago
Thanks. I'll add the parameter asserts and remove the old comment.

ALOG_BRIDGE is used in AndroidBridge.cpp. Here is one example, the source of the "empty body" warnings with the previous definition of ALOG_BRIDGE:

https://hg.mozilla.org/mozilla-central/file/19f630648c80/widget/android/AndroidBridge.cpp#l196
Oh, you're right, it is used -- not sure how I missed that before. I must've been searching the wrong file, or something.  So, nevermind -- disregard the ALOG_BRIDGE comment. :)
CC'ing gps & Ms2ger, to give them a heads-up about this bug's Makefile tweaks for the upcoming Makefile nuke/switchover.
You need to log in before you can comment on or make changes to this bug.