recent cache checkin upped warning count by 5

RESOLVED WORKSFORME

Status

()

P2
normal
RESOLVED WORKSFORME
16 years ago
7 years ago

People

(Reporter: timeless, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.3alpha
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
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.");

Updated

16 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.3alpha

Updated

16 years ago
Blocks: 187528

Updated

16 years ago
Depends on: 187530

Comment 1

16 years ago
Created attachment 115699 [details] [diff] [review]
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

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

timeless: Would you like to give this one an r=?
Attachment #115699 - Flags: superreview?(darin)
Attachment #115699 - Flags: review?
(Reporter)

Comment 3

16 years ago
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.
Attachment #115699 - Flags: review? → review+

Comment 4

16 years ago
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

16 years ago
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.  :-(
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → WONTFIX

Comment 6

16 years ago
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")?
Status: RESOLVED → REOPENED
OS: Windows 2000 → All
Resolution: WONTFIX → ---

Comment 7

16 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → WONTFIX

Comment 8

15 years ago
VERIFIED/worntfix.
Status: RESOLVED → VERIFIED

Comment 9

15 years ago
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!
Attachment #115699 - Flags: superreview?(darin) → superreview+

Comment 10

15 years ago
Great, I'll check it in the next chance I get.
Status: VERIFIED → REOPENED
Resolution: WONTFIX → ---
Assignee: gordon → nobody
QA Contact: tever → networking.cache

Comment 11

7 years ago
There is no warning in nsDiskCacheDevice.cpp file in tip of m-c. Closing this bug.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.