Enable FAIL_ON_WARNINGS in more of netwerk/

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Ms2ger, Assigned: valentin)

Tracking

(Blocks 1 bug)

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Posted patch Patch v1 (obsolete) — Splinter Review
There you go
Attachment #614877 - Flags: review?(jduell.mcbugs)
Reporter

Updated

7 years ago
Blocks: buildwarning

Comment 1

7 years ago
Comment on attachment 614877 [details] [diff] [review]
Patch v1

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

A couple nits.

Have you run through try?

::: netwerk/base/src/nsFileStreams.h
@@ +182,5 @@
>  {
>  public:
> +    using nsFileInputStream::Init;
> +    using nsFileInputStream::Read;
> +

You needed to add these to get rid of a warning?

::: netwerk/socket/nsSOCKSIOLayer.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Bonus points if you add a vim modeline here too.

Comment 2

7 years ago
Comment on attachment 614877 [details] [diff] [review]
Patch v1

+r with nits addressed.
Attachment #614877 - Flags: review?(jduell.mcbugs) → review+
Reporter

Comment 3

7 years ago
(In reply to Jason Duell (:jduell) from comment #1)
> Comment on attachment 614877 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 614877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple nits.
> 
> Have you run through try?
> 
> ::: netwerk/base/src/nsFileStreams.h
> @@ +182,5 @@
> >  {
> >  public:
> > +    using nsFileInputStream::Init;
> > +    using nsFileInputStream::Read;
> > +
> 
> You needed to add these to get rid of a warning?

Yeah... C++ hiding stuff

Updated

7 years ago
Depends on: 750445

Comment 4

7 years ago
Posted patch v1, part 2 (obsolete) — Splinter Review
Together with part 1 and my patch for 750445, this gets rid of all necko warnings for me, at least on my Linux box. Delegating review to ms2ger since he's been working on this stuff.

Pushed to try:

   https://tbpl.mozilla.org/?tree=Try&rev=be0f811a6c4d
Attachment #619770 - Flags: review?(Ms2ger)

Comment 5

7 years ago
Posted patch v1, part 3 (obsolete) — Splinter Review
I should have said, "gets rid of all necko warnings for me in directories with FAIL_ON_WARNINGS".  This gets rid of one of the last remaining warnings in the other directories, and turns on FAIL_ON_WARNINGS for all directories that contain C++ code except wifi/ and tests/

re: ParseFTPList.  C programmers do silly things sometimes.  Like use a pointer to a function as a boolean "isInitialized" flag when they could just use a contructor.

The last remaining necko warning (on linux at least) is in wifi/: it's harder to fix because we're using dlsym() to get a function pointer, but dlsym() itself returns void *, so there's no easy way to get rid of the warning about converting void * to a function ptr AFACIT (bug 703121 comment 8 suggests we may be able to do it at some point with a #pragma).  I'm not going to worry about it for now. 

This also fixes the OSX-only gcc warning about empty while statements that failed on my last try attempt.  Pushing again:

   https://tbpl.mozilla.org/?tree=Try&rev=f54d408a48d2
Assignee: Ms2ger → jduell.mcbugs
Attachment #619803 - Flags: review?

Updated

7 years ago
Attachment #619803 - Flags: review? → review?(Ms2ger)

Updated

7 years ago
Duplicate of this bug: 666617

Updated

7 years ago
Duplicate of this bug: 491190

Updated

7 years ago
Blocks: 750584

Updated

7 years ago
Blocks: 750616

Comment 8

7 years ago
filed bug 750616 to deal with android-only warning.
No longer blocks: 750616
Depends on: 750616

Comment 9

7 years ago
Posted patch part3, v2 (obsolete) — Splinter Review
updated to fix some OSX-only warnings.
Attachment #619803 - Attachment is obsolete: true
Attachment #619803 - Flags: review?(Ms2ger)
Attachment #619846 - Flags: review?(Ms2ger)

Comment 10

7 years ago
Honza:  This patch just disables your diagnostic check from bug 621446 on OSX.  Since we're only seeing the error on Linux, I assume that's OK.

With the four patches in this bug, plus those for bug 750445 and bug 750616, I'm now seeing all-green on try with FAIL_ON_WARNINGS turned all for all of necko except system/mac (I filed bug 750584 for that) and the dlsym issue in wifi/

   https://tbpl.mozilla.org/?tree=Try&rev=9d59ea146fb7
Attachment #619847 - Flags: review?(honzab.moz)
Comment on attachment 619770 [details] [diff] [review]
v1, part 2

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

::: netwerk/base/src/nsURLParsers.cpp
@@ +503,5 @@
>  
>      // search backwards for @
>      const char *p = auth + authLen - 1;
> +    for (; (*p != '@') && (p > auth); --p) {
> +      ;

Nit: I prefer 'continue;'

::: netwerk/cache/nsDiskCacheBinding.cpp
@@ +339,3 @@
>  
>      hashEntry = (HashTableEntry*) PL_DHashTableOperate(&table,
> +                                                       (void*)(PRUptrdiff) key,

key already is a void*, so just remove the casts here.
Attachment #619770 - Flags: review?(Ms2ger) → review+
Comment on attachment 619847 [details] [diff] [review]
part 4: avoid pragma warning on OSX

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

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +379,5 @@
>    return true;
>  }
>  
>  // Bug 621446 investigation
> +#if !defined(XP_MACOSX)

#ifdef ANDROID ?

Comment 13

7 years ago
> #ifdef ANDROID ?

I don't understand--you've added these pragmas for e10s, and Android isn't running e10s.
(In reply to Jason Duell (:jduell) from comment #13)
> > #ifdef ANDROID ?
> 
> I don't understand--you've added these pragmas for e10s, and Android isn't
> running e10s.

??? The crash is reported exceptionally on Fennec.  There is ANDROID defined, isn't it?

Was ever e10s used on Android?  Has that been changed recently?

If that is so, then I'm totally confused now..
The old XUL version of Fennec used e10s. The newer native version does not.
Josh, so this is INVALID on the native build?
I don't know what "this" is, unfortunately.
this == this bug
Oh.. not this bug, but bug 621446.
B2G is actively working on moving to e10s.
Josh, problem is that I now can't get any crash-debug data.  So the investigation patch is actually useless if we want bug 621446 get fixed before publish of b2g.

(In reply to Jason Duell (:jduell) from comment #13)
> > #ifdef ANDROID ?
> 
> I don't understand--you've added these pragmas for e10s, and Android isn't
> running e10s.

I want to be just symmetric.  We change the prefs when ANDROID is defined, so we should reset them again only when ANDROID is defined.

According the native fennec situation I think I'll back the investigation patch out and fix the crash by added null check+assertion+log.
Comment on attachment 619846 [details] [diff] [review]
part3, v2

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

LGTM

::: netwerk/cookie/nsCookieService.cpp
@@ +2852,5 @@
>    // remove trailing <LWS>; first check we're not at the beginning
>    lastSpace = aIter;
>    if (lastSpace != start) {
> +    while (--lastSpace != start && iswhitespace(*lastSpace))
> +      ;

continue here too
Attachment #619846 - Flags: review?(Ms2ger) → review+
Comment on attachment 619847 [details] [diff] [review]
part 4: avoid pragma warning on OSX

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

I'm completely removing this code in bug 621446.
Attachment #619847 - Flags: review?(honzab.moz)

Updated

7 years ago
Depends on: 621446

Updated

7 years ago
Attachment #619847 - Attachment is obsolete: true

Updated

7 years ago
Assignee: jduell.mcbugs → valentin.gosu

Comment 24

7 years ago
Valentin:  I think this should be able to land, once you compile again to check that we haven't added any new warnings since the patches were posted.

IIRC these patches cover the rest of necko--check to make sure I didn't miss any.

It'll be nice to have this!
Duplicate of this bug: 750584
Posted patch Final fix (obsolete) — Splinter Review
This patch fixes a remaining warning an enables FAIL_ON_WARNINGS = 1 for most of the subfolders. Skipped netwerk/locales and netwerk/protocol/app because they don't contain any C++ files.
Also enabled in netwerk/test, although I don't know if that's necessary.

Fixes warning:
In file included from /Users/vgosu/ff_trunk_debug/netwerk/base/src/nsFileStreams.cpp:21:
/Users/vgosu/ff_trunk_debug/netwerk/base/src/nsFileStreams.h:135:9: error: field 'mIOFlags' will be initialized after field 'mLineBuffer' [-Werror,-Wreorder]
      : mIOFlags(0), mLineBuffer(nullptr), mPerm(0), mCachedPosition(0)
        ^
1 error generated.
Attachment #654554 - Flags: review?(jduell.mcbugs)

Comment 27

7 years ago
Valentin:   the patches here duplicated a bunch of stuff, and some makefile infrastructure (and PR types changes) bitrotted them.  I fixed all that, but it still doesn't compile w/o warnings.   Grab this patch and try to compile, and fix the remaining warnings when you have time--thanks!
Attachment #614877 - Attachment is obsolete: true
Attachment #619770 - Attachment is obsolete: true
Attachment #619846 - Attachment is obsolete: true
Attachment #654554 - Attachment is obsolete: true
Attachment #654554 - Flags: review?(jduell.mcbugs)
Attachment #655121 - Flags: review-
Posted patch PatchSplinter Review
Passes try: https://tbpl.mozilla.org/?tree=Try&rev=7246d906a5e2

This patch adds FAIL_ON_WARNINGS to all folders that contain C++, except for netwerk/wifi and netwerk/streamconv/src on Mac, for the following failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=14697410&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=14696913&tree=Try
Attachment #655121 - Attachment is obsolete: true
Attachment #655336 - Flags: review?(jduell.mcbugs)
Attachment #655336 - Flags: feedback?(valentin.gosu)
Comment on attachment 655336 [details] [diff] [review]
Patch

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

Looks pretty good.

::: netwerk/Makefile.in
@@ +6,5 @@
>  DEPTH		= @DEPTH@
>  topsrcdir	= @top_srcdir@
>  srcdir		= @srcdir@
>  VPATH		= @srcdir@
> +FAIL_ON_WARNINGS := 1

This line actually does nothing
Attachment #655336 - Flags: feedback+

Comment 30

7 years ago
Comment on attachment 655336 [details] [diff] [review]
Patch

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

\o/  No more unnoticed warnings...
 
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a3c81ae4cabb
  
Landed w/o FAIL_ON_WARNINGS for /netwerk/cache due to new warning that landed there: filed bug 786154 for it.

::: netwerk/Makefile.in
@@ +6,5 @@
>  DEPTH		= @DEPTH@
>  topsrcdir	= @top_srcdir@
>  srcdir		= @srcdir@
>  VPATH		= @srcdir@
> +FAIL_ON_WARNINGS := 1

True.  I removed it.

::: netwerk/mime/nsMIMEHeaderParamImpl.cpp
@@ -795,5 @@
>        delimiters++;
> -    } else if (tc >= 128) {
> -      // fail early, not ASCII
> -      NS_WARNING("non-US-ASCII character in RFC5987-encoded param");
> -      return NS_ERROR_INVALID_ARG;

We want to keep the check for not-ASCII here. C++ allows compilers to choose whether 'char' is unsigned or not by default), so I changed the 'if' test to (((unsigned char)tc) >= 128).
Attachment #655336 - Flags: review?(jduell.mcbugs)
Attachment #655336 - Flags: review+
Attachment #655336 - Flags: feedback?(valentin.gosu)
https://hg.mozilla.org/mozilla-central/rev/a3c81ae4cabb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.