Can we get rid of NS_COMFALSE?

RESOLVED FIXED

Status

()

Core
XPCOM
P3
normal
RESOLVED FIXED
19 years ago
16 years ago

People

(Reporter: brendan, Assigned: sicking)

Tracking

({helpwanted})

Trunk
helpwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M10
(Reporter)

Updated

19 years ago

Comment 1

19 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

19 years ago
Target Milestone: M10 → M11

Updated

19 years ago
Assignee: dp → brendan
Status: ASSIGNED → NEW

Comment 2

19 years ago
Hey I wont have time to track/do this one. Assigning back to brendan. Anyway
this is the tracking bug.
(Reporter)

Updated

19 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

19 years ago
Target Milestone: M11 → M14

Comment 3

19 years ago
I have the time and desire, so I'm both tracking and implementing this one, in
stages.
(Reporter)

Updated

19 years ago
Assignee: brendan → braddr
Status: ASSIGNED → NEW

Comment 4

19 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

19 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

19 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 7

18 years ago
How come this is still M14? M14 is already out!

Comment 8

18 years ago
Mass moving old milestone bugs to M16
Target Milestone: M14 → M16

Comment 9

18 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

18 years ago
M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 11

18 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

18 years ago
dp is no longer @netscape.com. changing qa contact to default for this product
QA Contact: dp → kandrot

Comment 13

18 years ago
Milestone 0.8 has been released. We should either resolve this bug or update its
milestone.

Updated

18 years ago
Target Milestone: M18 → ---
*** Bug 11592 has been marked as a duplicate of this bug. ***
*** Bug 11593 has been marked as a duplicate of this bug. ***
*** Bug 11595 has been marked as a duplicate of this bug. ***
*** Bug 11597 has been marked as a duplicate of this bug. ***
*** Bug 169088 has been marked as a duplicate of this bug. ***
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
Created attachment 107233 [details] [diff] [review]
patch v1

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.
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 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+
> > -  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
yes, all the changed/removed comments were lying
Attachment #107233 - Flags: review?(peterv) → review+
checked in! thanks for reviews
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.