Closed Bug 970271 Opened 6 years ago Closed 6 years ago

[Network] Fix some GCC warnings in Necko

Categories

(Core :: Networking, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Details

Attachments

(5 files, 6 obsolete files)

2.38 KB, patch
vchang
: review+
Details | Diff | Splinter Review
1.98 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
2.27 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
1.43 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
5.61 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
GCC prints lots of warnings when bulding Necko code for Gonk.
Attachment #8373285 - Flags: review?(bzbarsky)
Comment on attachment 8373285 [details] [diff] [review]
[01] Bug 970271: Fix whitespace errors

Going to pass these off to someone actively involved in necko...
Attachment #8373285 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
Attachment #8373286 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
Attachment #8373287 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
Attachment #8373289 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
Attachment #8373290 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
Attachment #8373291 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
Attachment #8373292 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 8373285 [details] [diff] [review]
> [01] Bug 970271: Fix whitespace errors
> 
> Going to pass these off to someone actively involved in necko...

Oops, sorry. You are listed as a module peer for Necko.
> Oops, sorry. You are listed as a module peer for Necko.

Yeah, that's sort of a relic of times past.  I actually asked to be removed recently, but it hadn't happened yet...  Fixed now.
Comment on attachment 8373285 [details] [diff] [review]
[01] Bug 970271: Fix whitespace errors

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

Wow--epic amount of whitespace fixing! :)

So the general policy is this:  we're happy to take whitespace fixes that don't clutter the hg blame output.  So all the fixes here that fix space on otherwise empty lines (or lines that only have '}', etc) are fine.  

Lines that have source code we should just let sit, so if someone wants to use blame to track down why the code is so, they don't have to hop around version history to find it.   So strip those out of the patch, please.
Attachment #8373285 - Flags: review?(jduell.mcbugs) → feedback+
Attachment #8373286 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8373285 [details] [diff] [review]
[01] Bug 970271: Fix whitespace errors

Let's make this obsolete. These changes were done automatically by my text editor. I don't feel like going through all the files and sort out the empty-line fixes by hand. :D
Attachment #8373285 - Attachment is obsolete: true
I don't actually mind formatting cleanups that address more than empty lines. I'd rather wait for the big clang formatting patch being discussed on dev-mumble though

a] so this only happens once
b] so there is a tool that can enforce the style as part of checkin

also, and this is more important imo, rtsp and sctp changes should be looked at by :sworkman and :jesup.. they may involve things that are really just imported from upstream and need to be managed differently.
Comment on attachment 8373289 [details] [diff] [review]
[04] Bug 970271: Fix GCC warnings about ignored return value

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

Just a minor nit.

(Patrick: the Rtsp Parent/Child code is all Mozilla and not upstream.  I'll make sure to request additional review for rtsp/sctp stuff that might be upstream.)

::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
@@ +22,5 @@
>  #define SEND_DISCONNECT_IF_ERROR(rv)                         \
>    if (NS_FAILED(rv) && mIPCOpen && mTotalTracks > 0ul) {     \
>      for (uint32_t i = 0; i < mTotalTracks; i++) {            \
> +      bool success = SendOnDisconnected(i, rv);              \
> +      NS_WARN_IF_FALSE(success, "SendOnDisconnected failed");\

Do it this way, just for consistency with the rest of our e10s code:

  #include "mozilla/unused.h"

  unused << SendOnDisconnected(i, rv);

see for instance:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#608
Attachment #8373289 - Flags: review?(jduell.mcbugs) → feedback+
Comment on attachment 8373290 [details] [diff] [review]
[05] Bug 970271: Fix casting pointer to integer of different size

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

Looks fine but checking with Jesup to see if there's any upstream issue here.
Attachment #8373290 - Flags: review?(jduell.mcbugs) → review?(rjesup)
Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

ditto
Attachment #8373291 - Flags: review?(jduell.mcbugs) → review?(rjesup)
Comment on attachment 8373292 [details] [diff] [review]
[07] Bug 970271: Fix inaccessible base class nsIStreamListener

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

I don't understand this patch.  Can you clarify on what line of code GCC issues the error?

Does this code link once you change nsBaseChannel to inherit from nsIStreamListener?  You haven't implemented the nsIStreamListener methods.  And you don't want to :)

If there's any way we can fix this in RtspChannel that would be better. (Why can't GCC access the base nsIStreamListener class?  We're inheriting publicly from it).
Attachment #8373292 - Flags: review?(jduell.mcbugs) → review-
Comment on attachment 8373287 [details] [diff] [review]
[03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values

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

+r on the RtspControllerChild/Parent parts.   Forwarding the sctp bit to jesup
Attachment #8373287 - Flags: review?(jduell.mcbugs) → review?(rjesup)
Comment on attachment 8373287 [details] [diff] [review]
[03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values

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

r+, but note that the sctp source files are all from upstream, so you need to request a change in the SCTP upstream from Michael Tuexen (if they're not already fixed)
Attachment #8373287 - Flags: review?(rjesup) → review+
Michael, can you check if these are fixed upstream?  (I'll be importing an update soon.)  Thanks
Flags: needinfo?(tuexen)
(In reply to Randell Jesup [:jesup] from comment #20)
> Michael, can you check if these are fixed upstream?  (I'll be importing an
> update soon.)  Thanks

Will do tomorrow and let you know.

Best regards
Michael
Randell,

most of them were already addressed, mostly in a different way. The ones which
were not addressed have been addressed in
https://code.google.com/p/sctp-refimpl/source/detail?r=8809
https://code.google.com/p/sctp-refimpl/source/detail?r=8810

So all issues should be resolved by updating the usrsctp code.

Best regards
Michael
Flags: needinfo?(tuexen)
Comment on attachment 8373290 [details] [diff] [review]
[05] Bug 970271: Fix casting pointer to integer of different size

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

Ok, since these are resolved upstream, I'm going to cancel review of them for the SCTP code and it will be resolved when I import a new SCTP library shortly.
Attachment #8373290 - Flags: review?(rjesup)
Comment on attachment 8373287 [details] [diff] [review]
[03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values

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

please don't land the sctp parts per above
Changes to v1:

  - removed SCTP fixes
Attachment #8373286 - Attachment is obsolete: true
Attachment #8380721 - Flags: review+
Changes to v1:

  - removed SCTP fixes
Attachment #8373287 - Attachment is obsolete: true
Attachment #8380722 - Flags: review+
Changes to v1:

  - shift by return value, as shown in comment 14
Attachment #8373289 - Attachment is obsolete: true
Attachment #8380729 - Flags: review?(jduell.mcbugs)
Comment on attachment 8373290 [details] [diff] [review]
[05] Bug 970271: Fix casting pointer to integer of different size

Obsoleting [05] as it only contains SCTP fixes.
Attachment #8373290 - Attachment is obsolete: true
(In reply to Jason Duell (:jduell) from comment #17)
> Comment on attachment 8373292 [details] [diff] [review]
> [07] Bug 970271: Fix inaccessible base class nsIStreamListener
> 
> Review of attachment 8373292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand this patch.  Can you clarify on what line of code GCC
> issues the error?

I see this on the command line:

> In file included from /home/mozilla/Projects/mozilla/src/mozilla-central/netwerk/protocol/rtsp/RtspChannel.cpp:8:
/home/mozilla/Projects/mozilla/src/mozilla-central/netwerk/protocol/rtsp/RtspChannel.h:26: warning: direct base 'nsIStreamListener' inaccessible in 'mozilla::net::RtspChannel' due to ambiguity
> In file included from /home/mozilla/Projects/mozilla/src/mozilla-central/netwerk/protocol/rtsp/RtspHandler.cpp:8:
/home/mozilla/Projects/mozilla/src/mozilla-central/netwerk/protocol/rtsp/RtspChannel.h:26: warning: direct base 'nsIStreamListener' inaccessible in 'mozilla::net::RtspChannel' due to ambiguity

> Does this code link once you change nsBaseChannel to inherit from
> nsIStreamListener?  You haven't implemented the nsIStreamListener methods. 
> And you don't want to :)

Yes it links fine.

RtspChannel inherits from nsBaseChannel. The ambiguity comes from RtspChannel and nsBaseChannel both inheriting from nsIStreamListener. I removed the inheritance in RtspChannel, and made the inheritance of nsBaseChannel protected, so that RtspChannel can access the implementation.

> If there's any way we can fix this in RtspChannel that would be better. (Why
> can't GCC access the base nsIStreamListener class?  We're inheriting
> publicly from it)
Attachment #8380729 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8373292 [details] [diff] [review]
[07] Bug 970271: Fix inaccessible base class nsIStreamListener

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

Ah, now I understand.  Nice fix--thanks!
Attachment #8373292 - Flags: review- → review+
Ping for review of [06].
Flags: needinfo?(rjesup)
Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

I'm not involved in RTSP.....  Check the logs for those files.
Attachment #8373291 - Flags: review?(rjesup)
Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

jduell and sworkman reviewed previous checkins to these files
Attachment #8373291 - Flags: review?(jduell.mcbugs)
Flags: needinfo?(rjesup)
Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

Benjamin, can you review these RTSP bits? (you're listed in hg log as the person who added the files).  The main issue is whether the code comes from upstream (and if so whether we should submit the fixes upstream instead of in our tree).
Attachment #8373291 - Flags: review?(jduell.mcbugs) → review?(bechen)
Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

Redirect to :vchang
Attachment #8373291 - Flags: review?(bechen) → review?(vchang)
Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

Thanks for your help on this.
Attachment #8373291 - Flags: review?(vchang) → review+
Changes to v2:

  - updated for removal of whitespace fix [01]
Attachment #8380722 - Attachment is obsolete: true
Attachment #8382860 - Flags: review+
Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

::: netwerk/protocol/rtsp/rtsp/AMPEG4AudioAssembler.cpp
@@ +106,5 @@
>  }
>  
>  static status_t parseGASpecificConfig(
>          ABitReader *bits,
>          unsigned audioObjectType, unsigned channelConfiguration) {

Hi Thomas,
Thank you for cleaning those annoying GCC warnings. Appreciated. :)

Removal of this line caused a regression in bug 979761.
ABitReader::getBits() actually shifts its internal members recording the bit position parsed.
I think we should follow the style in other places in this file, that is:
/*unsigned frameLengthFlag = */bits->getBits(1);

This is fixed in the patch in bug 979761.
Oh, sorry about this and thanks a lot for fixing the problem. I'll be more careful in the future.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #42)
> Oh, sorry about this and thanks a lot for fixing the problem. I'll be more
> careful in the future.
Don't be sorry. I am so happy to have these fixes to eliminate warnings.
It just reminds me that we should hurry up to work out a test framework of the RTSP feature, so that we can prevent such regression bug.
You need to log in before you can comment on or make changes to this bug.