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

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect, P4)

Description

[Chris Peterson [:cpeterson]]

Attachment: 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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 706269 [details] [diff] [review]
fix-android-warnings

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

LGTM

Comment 2

[Daniel Holbert [:dholbert]]

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).

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 4

[Daniel Holbert [: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!

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Chris Peterson [:cpeterson]]

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

Comment 7

[Daniel Holbert [:dholbert]]

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. :)

Comment 8

[Daniel Holbert [:dholbert]]

CC'ing gps & Ms2ger, to give them a heads-up about this bug's Makefile tweaks for the upcoming Makefile nuke/switchover.

Comment 9

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/874b9c8e05f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9012158b65f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/185ab4996e2d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/874b9c8e05f1
https://hg.mozilla.org/mozilla-central/rev/9012158b65f6
https://hg.mozilla.org/mozilla-central/rev/185ab4996e2d