Closed Bug 161413 Opened 18 years ago Closed 8 years ago

recent cache checkin upped warning count by 5

Categories

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

x86
All
defect

Tracking

()

RESOLVED WORKSFORME
mozilla1.3alpha

People

(Reporter: timeless, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.");
Priority: -- → P2
Target Milestone: --- → mozilla1.3alpha
Blocks: buildwarning
Depends on: 187530
This fixes the last of the listed warnings.  The others have been taken care of
in previous checkins.
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?
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 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.
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
Closed: 17 years ago
Resolution: --- → WONTFIX
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 → ---
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
Closed: 17 years ago17 years ago
Resolution: --- → WONTFIX
VERIFIED/worntfix.
Status: RESOLVED → VERIFIED
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+
Great, I'll check it in the next chance I get.
Status: VERIFIED → REOPENED
Resolution: WONTFIX → ---
Assignee: gordon → nobody
QA Contact: tever → networking.cache
There is no warning in nsDiskCacheDevice.cpp file in tip of m-c. Closing this bug.
Status: REOPENED → RESOLVED
Closed: 17 years ago8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.