Last Comment Bug 781399 - Fix warnings in networking code
: Fix warnings in networking code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla17
Assigned To: Valentin Gosu [:valentin]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-08 17:58 PDT by Valentin Gosu [:valentin]
Modified: 2012-08-09 19:58 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1.0 (1.30 KB, patch)
2012-08-08 18:10 PDT, Valentin Gosu [:valentin]
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Valentin Gosu [:valentin] 2012-08-08 17:58:42 PDT

    
Comment 1 Valentin Gosu [:valentin] 2012-08-08 18:10:49 PDT
Created attachment 650402 [details] [diff] [review]
Fix v1.0

https://tbpl.mozilla.org/?tree=Try&rev=cbc3e2e6ff8e
Comment 2 Jason Duell [:jduell] (needinfo me) 2012-08-08 20:48:57 PDT
Comment on attachment 650402 [details] [diff] [review]
Fix v1.0

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

Valentin:

I assume you want this reviewed?  If so it's usually a good idea to assign it to one of the necko module peers (list at https://wiki.mozilla.org/Modules/Core#Necko), else it will get forgotten.

I need more info to understand whether this patch actually gets rid of warnings--see below.  Also, you could probably mark this bug RESOLVED DUPLICATE of bug 745296 and add any more patches we need to get rid of necko warnings there.

::: netwerk/base/src/nsFileStreams.h
@@ +175,5 @@
>  class nsPartialFileInputStream : public nsFileInputStream,
>                                   public nsIPartialFileInputStream
>  {
>  public:
> +    using nsFileInputStream::Init;

What warning does this get rid of?  I'm used to this sort of thing causing a compiler error rather than a warning...

::: netwerk/wifi/osx_corewlan.mm
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #import <Cocoa/Cocoa.h>
> +#import <CoreWLAN/CoreWLAN.h>

Same here--I'd expect an #include to get rid of an error, but rarely fix a warning.
Comment 3 Josh Aas 2012-08-08 22:04:08 PDT
I suggested to Valentin that he wait for a green try run before requesting review.

I can confirm that these fixes do fix warnings, at least on Mac OS X with clang.

The first one is a hidden-via-overload warning for the "Init" symbol. There are methods named that in nsFileINputStream and nsIPartialFileInputStream, though with different signatures. What he did un-hides the symbol and the warning goes away.

As for the second one... An Obj-C method call (really a message) to a method that doesn't necessarily exist isn't fatal at compile-time. It's just a warning, basically because it's a message not a function call.  In this case, the methods in question weren't publicly exposed on OS X 10.5 so we just made the calls anyway and they worked. Now that we don't support 10.5 we can depend on the CoreWLAN framework headers existing (10.6+), so we can just include them and kill off the warnings.

I hope this is a clear enough explanation! I may be typing a bit too quickly atm.
Comment 4 Josh Aas 2012-08-08 22:07:20 PDT
Comment on attachment 650402 [details] [diff] [review]
Fix v1.0

Try run looks fine, requesting review for Valentin.
Comment 5 Jason Duell [:jduell] (needinfo me) 2012-08-08 22:07:40 PDT
Comment on attachment 650402 [details] [diff] [review]
Fix v1.0

Sounds good.  +r assuming try is green (good luck getting try results in a web browser--I haven't been able to for last day).
Comment 6 Jason Duell [:jduell] (needinfo me) 2012-08-08 22:08:36 PDT
Comment on attachment 650402 [details] [diff] [review]
Fix v1.0

heh--this is what I get when I try to "discard" Josh's changes that he made while I was marking +r
Comment 7 Valentin Gosu [:valentin] 2012-08-08 22:47:10 PDT
Sorry for the late reply. I was out.
Josh explained everything quite well. (Thanks, Josh!)
Try is almost complete and mostly green. :)
Comment 8 Josh Aas 2012-08-09 09:50:33 PDT
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/e8e5b353ca9c
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-09 19:58:27 PDT
https://hg.mozilla.org/mozilla-central/rev/e8e5b353ca9c

Note You need to log in before you can comment on or make changes to this bug.