Closed
Bug 8929
Opened 25 years ago
Closed 22 years ago
Can we get rid of NS_COMFALSE?
Categories
(Core :: XPCOM, defect, P3)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: brendan, Assigned: sicking)
References
Details
(Keywords: helpwanted)
Attachments
(1 file)
38.87 KB,
patch
|
peterv
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
As waterson and warren demonstrated by fisticuffs, NS_COMFALSE overloading of the return value makes a hazard: nsresult rv = ifp->someMethodReturningNS_COMFALSE(); if (rv == NS_COMFALSE) ok, false was returned; else true? could be error! Why not axe NS_COMFALSE now and never have this hazard darken our door? /be
Updated•25 years ago
|
Status: NEW → ASSIGNED
Target Milestone: M10
Reporter | ||
Updated•25 years ago
|
Comment 1•25 years ago
|
||
Ok, I'm making this the meta-bug (bug about bugs? hmm, it's sort of that, but it is also about NS_COMFALSE -- maybe uber- rather than meta-). /be
Updated•25 years ago
|
Target Milestone: M10 → M11
Updated•25 years ago
|
Assignee: dp → brendan
Status: ASSIGNED → NEW
Comment 2•25 years ago
|
||
Hey I wont have time to track/do this one. Assigning back to brendan. Anyway this is the tracking bug.
Reporter | ||
Updated•25 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•25 years ago
|
Target Milestone: M11 → M14
Comment 3•25 years ago
|
||
I have the time and desire, so I'm both tracking and implementing this one, in stages.
Reporter | ||
Updated•25 years ago
|
Assignee: brendan → braddr
Status: ASSIGNED → NEW
Comment 4•25 years ago
|
||
Is NS_COMFALSE ever going to really go away? I just got burned because fur changed nsHashtableEnumerator::IsDone to return NS_COMFALSE to 'be consistent with other enumerators' without fixing the code that uses it. This is a step backwards. I see that we now have NS_ENUMERATOR_FALSE (which equals 1 like NS_COMFALSE) all over the place. http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIEnumerator.idl#26 http://lxr.mozilla.org/seamonkey/ident?i=NS_ENUMERATOR_FALSE
Comment 5•25 years ago
|
||
NS_ENUMERATOR_FALSE is one step along the way of eliminating NS_COMFALSE from the rest of the code. The bad usage among nsIEnumerators is the largest consumer and one of the more complicated to get rid of. With that one out of the way for now, the rest of the usages are easier to remove from the code and that's what I've been doing, if incredibly slowly. The right answer for enumerators is to NOT use NS_ENUMERATOR_FALSE or NS_COMFALSE, its to have a function like ::isDone(bool myResult) -- or something along those lines, NOT overloading the nsresult with both error codes and the true/false result its trying to convey. Make sense/
Status: NEW → ASSIGNED
Comment 6•25 years ago
|
||
I'm glad you're dealing with this. I don't really see how transitioning to NS_ENUMERATOR_FALSE really helps much. Whacking a tree to switch all declarations of these methods to use the boolean out param is the only way you are ever going to uncover and fix all the insidious little uses of this pattern.
Comment 9•24 years ago
|
||
This is pretty old so I'm doing some Cleaning up Up to date lists submitted to open bugs bug 11592 , bug 11593 , bug 11595 . bug 11597 can be resolved fixed. In addition to open bugs NS_COMFALSE was found in /js/src/xpconnect/xpccomponents.cpp /mailnews/db/msgdb/public/public/nsIDBChangeAnnouncer.idl /mailnews/db/msgdb/src/msMsgDatabase.cpp so bug 11591 and 11598 need to be reopened as regressions.
Comment 10•24 years ago
|
||
M16 has been out for a while now, these bugs target milestones need to be updated.
Comment 11•24 years ago
|
||
moving this out a few. I don't have the time for this, if someone else does, grab it, run with it, maybe even finish it.
Keywords: helpwanted
Target Milestone: M16 → M18
Comment 12•24 years ago
|
||
dp is no longer @netscape.com. changing qa contact to default for this product
QA Contact: dp → kandrot
Comment 13•24 years ago
|
||
Milestone 0.8 has been released. We should either resolve this bug or update its milestone.
Updated•24 years ago
|
Target Milestone: M18 → ---
Assignee | ||
Comment 14•22 years ago
|
||
*** Bug 11592 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 15•22 years ago
|
||
*** Bug 11593 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 16•22 years ago
|
||
*** Bug 11595 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 17•22 years ago
|
||
*** Bug 11597 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 18•22 years ago
|
||
*** Bug 169088 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 19•22 years ago
|
||
I have a patch for this in bug 169088 that i'll sync to tip in a little while
Assignee: braddr → bugmail
Status: ASSIGNED → NEW
Assignee | ||
Comment 20•22 years ago
|
||
Fixes comments from peterv and dbaron. The patch still does what is described in bug 169088 comment 6. The only difference is that i've split up nsContentErrors.h in to nsContentErrors.h and nsLayoutErrors.h and placed those files in {content|layout}/base/public. I've also given a proper name to the error-constant.
Assignee | ||
Comment 21•22 years ago
|
||
Comment on attachment 107233 [details] [diff] [review] patch v1 Peterv: This patch hasn't changed codewise since you reviewed it in bug 169088. The only real change is that nsContentErrors.h has been split up in two files
Attachment #107233 -
Flags: superreview?(bzbarsky)
Attachment #107233 -
Flags: review?(peterv)
Comment 22•22 years ago
|
||
Comment on attachment 107233 [details] [diff] [review] patch v1 > - return NS_COMFALSE; > + return NS_OK; This is in nsSelection.cpp. Does no one check the return value or something? nsGenericHTMLElement.cpp: > if (nsnull != mAttributes) { fix this while you're here? ;) I assume the comments on the Handle*Event functions were just out of touch with reality? sr=bzbarsky pending explanation on the nsSelection issue.
Attachment #107233 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 23•22 years ago
|
||
> > - return NS_COMFALSE; > > + return NS_OK; > This is in nsSelection.cpp. Does no one check the return value or something? No C++ caller of this function checks the returnvalue. The function is internal to the class so there are no JS-callers (and if there were they wouldn't see a difference since both are success-values)
Status: NEW → ASSIGNED
Assignee | ||
Comment 24•22 years ago
|
||
yes, all the changed/removed comments were lying
Updated•22 years ago
|
Attachment #107233 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 25•22 years ago
|
||
checked in! thanks for reviews
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•