161413 - recent cache checkin upped warning count by 5

Status: RESOLVED WORKSFORME

(Core :: Networking: Cache, defect, P2)

Description

[timeless]

http://tinderbox.mozilla.org/SeaMonkey/warn1028682120.14846.html#gordon
http://tinderbox.mozilla.org/SeaMonkey/warn1028685240.20418.html#gordon

3.netwerk/cache/src/nsDiskCacheDevice.cpp:597 (See build log excerpt)Unused
variable `nsresult rv'

595 if (!binding->mDoomed) {
596     // so it can't be seen by FindEntry() ever again.
597     nsresult rv = mCacheMap->DoomRecord(&binding->mRecord);
598     NS_ASSERTION(NS_SUCCEEDED(rv), "DoomRecord failed.");
599     binding->mDoomed = PR_TRUE; // record in no longer in cache map

4.netwerk/cache/src/nsDiskCacheStreams.cpp:714 (See build log excerpt)Comparison
between signed and unsigned

712 // write buffer
713 PRInt32 bytesWritten = PR_Write(mFD, mBuffer, mBufEnd);
714 if (bytesWritten != mBufEnd) {
715     NS_WARNING("failed to flush all data");
716     return NS_ERROR_UNEXPECTED;     // NS_ErrorAccordingToNSPR()

5.netwerk/cache/src/nsDiskCacheStreams.cpp:779 (See build log excerpt)Comparison
between signed and unsigned

777 if (!mBinding)  return NS_ERROR_NOT_AVAILABLE;
778 
779 if (offset > mStreamEnd)  return NS_ERROR_FAILURE;

6.netwerk/cache/src/nsDiskCacheStreams.cpp:851 (See build log excerpt)Comparison
between signed and unsigned

849 }
850 
851 if ((newPos < 0) || (newPos > mBufEnd)) {
852     NS_WARNING("seek offset out of range");
853     return NS_ERROR_INVALID_ARG;

7.netwerk/cache/src/nsMemoryCacheDevice.cpp:254 (See build log
excerpt)Comparison between signed and unsigned

252 // we have the right to refuse or pre-evict
253 PRUint32  newSize = entry->DataSize() + deltaSize;
254 if (newSize > mSoftLimit) {
255     nsresult rv = nsCacheService::DoomEntry(entry);
256     NS_ASSERTION(NS_SUCCEEDED(rv),"DoomEntry() failed.");

Comment 1

[gordon]

Attachment: fix warning in nsDiskCacheDevice.cpp

This fixes the last of the listed warnings.  The others have been taken care of
in previous checkins.

Comment 2

[gordon]

Comment on attachment 115699 [details] [diff] [review]
fix warning in nsDiskCacheDevice.cpp

timeless: Would you like to give this one an r=?

Comment 3

[timeless]

Comment on attachment 115699 [details] [diff] [review]
fix warning in nsDiskCacheDevice.cpp

technically this changes the behavior for debug builds (you can't rely on
XPCOM_DEBUG_BREAK to break for this), but as long as you're ok with that.

Comment 4

[Darin Fisher]

Comment on attachment 115699 [details] [diff] [review]
fix warning in nsDiskCacheDevice.cpp

>Index: nsDiskCacheDevice.cpp

>-        NS_ASSERTION(NS_SUCCEEDED(rv), "DoomRecord failed.");
>+        if (NS_FAILED(rv)) {
>+            NS_WARNING("DoomRecord failed.");
>+        }

ok, so now you are depending on the compiler to eliminate the branch?  seems
like you'd really want this:

#ifdef DEBUG
    nsresult rv =
#endif
    DoomRecord(..);
    NS_ASSERTION(NS_SUCCEEDED(rv), "DoomRecord failed");

but that is oogly!  i'd just keep it as it was and ignore the opt-build
warning.  we have the same kind of opt-build warning in numerous other places.

Comment 5

[gordon]

Okay, after getting input from Darin, alecf, and brendan, I'm leaving it as it
is, and we'll just have to ignore the opt warnings.  Not a perfect solution, but
adding #ifdef DEBUG around the declaration and use of nsresult rv is ugly and
fragile.

Marking WONTFIX.  :-(

Comment 6

[Aleksey Nogin]

Have you looked at the bug 187530? It lists several options that are much less ugly.

But even if the first ("unused variable") warning is WONTFIX, what about the
other 4 ("comparison between signed and unsigned")?

Comment 7

[gordon]

In comment 1 I explained the other warnings were already fixed.  And in bug
187530, I posted a comment that I tried to use just such an approach and was
disuaded by multiple super-reviewers.

Marking WONTFIX, again.

Comment 8

[benc]

VERIFIED/worntfix.

Comment 9

[Darin Fisher]

Comment on attachment 115699 [details] [diff] [review]
fix warning in nsDiskCacheDevice.cpp

gordon: fwiw, i did some testing and actually the if branch without an empty
body will be compiled away when optimizations are enabled.  so, this patch is
perfectly fine afterall.  sorry!

Comment 10

[gordon]

Great, I'll check it in the next chance I get.

Comment 11

[Atul Aggarwal]

There is no warning in nsDiskCacheDevice.cpp file in tip of m-c. Closing this bug.