Closed Bug 11595 Opened 25 years ago Closed 22 years ago

eliminate NS_COMFALSE

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Platform:
All
Other
Type:
defect

Tracking

()

Status:
VERIFIED DUPLICATE of bug 8929
Milestone:
Future

People

(Reporter: warrensomebody, Assigned: attinasi)

References

Details

Attachments

(1 file, 1 obsolete file)

How about this patch guys.. I didn't expect so much debate of this tiny patch, but its good feedback. Any objections to this one?
2.74 KB, patch
Details | Diff | Splinter Review 
We should eliminate the NS_COMFALSE madness once and for all. Here's a list of
offending uses in your module -- please pass the bug along if there's someone
else who should deal with it.

      layout/base/src/nsDocument.cpp:
            View change log or Blame annotations
            line 2695
      layout/base/src/nsFrameTraversal.cpp:
            View change log or Blame annotations
            line 175
            line 186
      layout/base/src/nsRangeList.cpp:
            View change log or Blame annotations
            line 426
            line 1471
            line 1577
      layout/base/src/nsStyleSet.cpp:
            View change log or Blame annotations
            line 766
      layout/base/src/nsContentIterator.cpp:
            View change log or Blame annotations
            line 680
      layout/base/src/nsRange.cpp:
            View change log or Blame annotations
            line 799
            line 1176
            line 1518
      layout/base/src/nsSpaceManager.cpp:
            View change log or Blame annotations
            line 131
            line 277
            line 628
      layout/base/tests/TestSpaceManager.cpp:
            View change log or Blame annotations
            line 731
      layout/html/base/src/nsBlockFrame.cpp:
            View change log or Blame annotations
            line 897
      layout/html/base/src/nsContainerFrame.cpp:
            View change log or Blame annotations
            line 165
      layout/html/base/src/nsFrame.cpp:
            View change log or Blame annotations
            line 516
            line 544
      layout/html/content/src/nsGenericHTMLElement.cpp:
            View change log or Blame annotations
            line 1017
      layout/html/forms/src/nsComboboxControlFrame.cpp:
            View change log or Blame annotations
            line 880
      layout/html/forms/src/nsHTMLButtonControlFrame.cpp:
            View change log or Blame annotations
            line 596
      layout/html/forms/src/nsGfxRadioControlFrame.cpp:
            View change log or Blame annotations
            line 76
      layout/html/style/src/nsCSSFrameConstructor.cpp:
            View change log or Blame annotations
            line 5180
            line 5262
            line 5501
      layout/html/style/src/nsCSSLoader.cpp:
            View change log or Blame annotations
            line 1180
            line 1233
      layout/html/style/src/nsCSSParser.cpp:
            View change log or Blame annotations
            line 856
      layout/html/style/src/nsCSSRules.cpp:
            View change log or Blame annotations
            line 601
            line 651
      layout/html/style/src/nsCSSStyleSheet.cpp:
            View change log or Blame annotations
            line 1372
            line 1854
            line 1962
            line 2067
            line 2453
      layout/html/style/src/nsHTMLAttributes.cpp:
            View change log or Blame annotations
            line 1370
      layout/html/style/src/nsHTMLCSSStyleSheet.cpp:
            View change log or Blame annotations
            line 523
      layout/html/style/src/nsHTMLStyleSheet.cpp:
            View change log or Blame annotations
            line 732
      layout/xul/base/src/nsProgressMeterFrame.cpp:
            View change log or Blame annotations
            line 771
      layout/xul/base/src/nsTitledButtonFrame.cpp:
            View change log or Blame annotations
            line 1188
      layout/xul/base/src/nsToolboxFrame.cpp:
            View change log or Blame annotations
            line 177
Blocks: 8929
Assignee: rickg → peterl
Peter - please address the issues shown here, then pass the is bug onto the next
guy on the list (based on file ownership). Thanks --
Rick
Pushing off non-beta 1 issues
Reassigning peterl's bugs to myself.
Accepting peterl's bugs that have a Target Milestone
Several of the files have been taken care of, more coming..
Thanks, braddr! Are you taking care of the issue across the board?
Pushing my M15 bugs to M16
Marc -- this is an easy one for you; no rush, but easy background work.
Assignee: pierre → attinasi
Status: ASSIGNED → NEW
Now I just have to find out what NS_COMFALSE is...
Status: NEW → ASSIGNED
Target Milestone: M16 → M20
Beautiful. Lovely. Moving. Check it in!
Isn't the sense of sheetModified backwards?  It's false when mDirty, true 
otherwise -- but a dirty bit is set when something's modified and needs to be 
"cleaned" (not to mix metaphors).  Did the boolean sense of IsUnmodified get 
inverted by mistake?

Nit, notwithstanding the above concern: !mDirty is shorter and better than 
mDirty ? false : true in my book.

I second warren's sentiments.  This NS_COMFALSE purging is most rigtheous.  
Thanks, braddr.

/be
I prefer 'mDirty ? PR_FALSE : PR_TRUE' to '!mDirty'. This way if a caller is 
testing on a specific value then it will match either way. !mDirty is not 
guaranteed to be equal to PR_TRUE when non-false.

in CSSStyleSheetImpl::IsUnmodified I think the new argument should be called 
sheetUnmodified, not sheetModified...
Unless it's too early and I need more caffeine, I don't believe "!mDirty is not 
guaranteed to be equal to PR_TRUE when non-false." is true, or what you meant 
(did you mean "when false" rather than "when non-false"?).  !mDirty is 1 if 
mDirty is 0, and 0 if mDirty is any non-zero value.  What's more, if mDirty is a 
PRBool, its values should never be other than 0 or 1 -- not that it matters for 
this argument.

(And not that C or our C-like use of PRBools and PRPackedBools in C++ code 
actually enforces it via strong typing, or anything. ;-)

(PR_TRUE is and must always be 1, and PR_FALSE 0; that's set in stone by the 
same laws of C that produce 1 from !0 and 0 from !1 and !42 and any other 
boolean-negated, non-zero value.  Wherefore the rare idiot b = !!x to convert a 
numeric-type value x to a C boolean.  Some low-level code on a new architecture 
that wants to use 0xffffffff for true might rather PR_TRUE were all-ones than 1, 
but it will never be.  The C++ bool-typed literal 'true' also might be targeted 
to all-ones on such an architecture, but we don't use C++ bool for portability 
reasons, and anyway, if you were converting from bool to int [aka PRBool], C++ 
would do the right thing and convert all-ones to 1.)

Anyway, I think braddr undid the double-negative I was whining about in my last 
update to this bug, by renaming the method IsModified and having it return 
mDirty's value via the out parameter.  Less useless boolean-inverse computation 
that way, as well as less-obfuscated-by-gratuitous-negation code.

/be
"Wherefore the rare idiot b = !!x to convert a numeric-type value x to a C 
boolean."

I meant to write "idiom", not "idiot" -- as with "kick the bucket", a phrase 
newcomers to English cannot parse at first, !!x doesn't immediately or obviously 
connote what the programmer means.  No offense to scc or others who may use this 
idiom, and I hope it never becomes a cliche'.

/be
brendan - you are correct. I have been laboring under the erroneous impression 
(probably from way back) that the logical negation operator only guaranteed a 
value of zero or non-zero. According to you and Bjarne it is either 0 or 1, 
PR_FALSE or PR_TRUE, so I retract that part of my previous comment - which was 
mistyped anyway.
Yikes!  Don't look at me.  I normally disapprove of automatic conversions to |
bool| in general (in the user-defined class case) since they can make comparisons 
silently do the wrong thing.  This is less likely for people who implement |
operator!()|, but now |bool| conversion is no longer symmetric.  I prefer 
explicit functions like |is_empty()| or |is_ok()| or |not_sleeping()|.  So the |
IsModified()| function is right in line with my personal philosophy.  I have 
never used the |!!| trick; and for the very reason Brendan cited: it's more 
clever than obvious.  In general, it's probably better to test for truth than to 
compare with |PR_TRUE|, e.g., prefer

  if ( x )

over

  if ( PR_TRUE == x )

The comparison, as noted earlier, isn't even guaranteed to work.  If you really 
felt a comparison was required, you must say this (but expect a spanking, |if 
(x)| is just absolutely the best thing to say)

  if ( PR_FALSE != PRBool(x) )
    // ...

In the case of a numeric value (which is what I think we're discussing here), 
where a |bool| conversion exists, better to say one of the following when needed 
to satisfy the type system:

  PRBool(x)
  NS_STATIC_CAST(PRBool, x)

If you _must_ produce a value that compares equal to either |PR_TRUE| or |
PR_FALSE|, e.g., as a return value, then use the |?:| operator, for example:

  x ? PR_TRUE : PR_FALSE

I guess this particular comment doesn't have a lot of direct impact on this bug, 
but I think it's stuff worth saying.

braddr -- looks good, only nit: the comment in nsICSSStyleSheet.h on the same
line as IsModified's declaration is stale: it talks about NS_COMFALSE vs. NS_OK
return value.

/be
Up To date list

/layout/base/public/nsIFrameSelection.h
        line 139 -- * will return NS_OK if it handles the event or NS_COMFALSE 
if not.
        line 147 -- * will return NS_OK if it handles the event or NS_COMFALSE 
if not.
/layout/base/public/nsISelection.h
        line 48 -- * will return NS_OK if it handles the event or NS_COMFALSE if 
not.
/layout/base/src/nsDocument.cpp
        line 3377 -- while (NS_COMFALSE == iter->IsDone())
/layout/base/src/nsStyleSet.cpp
        line 1053 -- return ((data.mStateful) ? NS_OK : NS_COMFALSE);
        line 1055 -- return NS_COMFALSE;
        line 2873 -- * will return NS_OK if it handles the event or NS_COMFALSE 
if not.
        line 3322 -- return NS_COMFALSE;
/layout/html/content/src/nsGenericHTMLElement.cpp
        line 1680 -- return NS_COMFALSE;
/layout/html/style/src/nsCSSRules.cpp
        line 755 -- return NS_COMFALSE;
/layout/html/style/src/nsCSSStyleSheet.cpp
        line 1691 -- return NS_COMFALSE;
        line 2898 -- if (NS_COMFALSE == 
styledContent->HasClass(classList->mAtom)) {
        line 3477 -- return ((isStateful) ? NS_OK : NS_COMFALSE);
/layout/html/style/src/nsHTMLAttributes.cpp
        line 1414 -- return NS_COMFALSE;
/layout/html/style/src/nsHTMLCSSStyleSheet.cpp
        line 602 -- return NS_COMFALSE;
/layout/html/style/src/nsHTMLStyleSheet.cpp
        line 961 -- nsresult result = NS_COMFALSE;
/layout/xul/base/src/nsScrollbarFrame.cpp
        line 169 -- return NS_COMFALSE;
This bug has been marked "future" because we have determined that it is not 
critical for netscape 6.0. If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.
Target Milestone: M20 → Future
I was actually doing the updates for 3 related bugs, bug 8929 , bug 11593 and
bug 11597 that are targeted for M-16. I just updated this one too while I was at it.

The previous comment is a 'canned' comment for bugs getting marked with the 
target milestone FUTURE - that's why it was added.

Please continue to update this bug - we *will* get to it and when we do the more 
up to date the information is the better. Thanks for the data!
Attachment #7571 - Attachment is obsolete: true

*** This bug has been marked as a duplicate of 8929 ***
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → DUPLICATE
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: