77323 - fancy use of double-negation (!!mInsideNoXXXTag) looks like typo

Status: RESOLVED WONTFIX

(Core :: DOM: HTML Parser, defect)

Description

[rbs]

776 /**
777  * Factory subroutine to create all of the html content objects.
778  */
779 nsresult
780 HTMLContentSink::CreateContentObject(const nsIParserNode& aNode,
781                                      nsHTMLTag aNodeType,
782                                      nsIDOMHTMLFormElement* aForm,
783                                      nsIWebShell* aWebShell,
784                                      nsIHTMLContent** aResult)
785 {
...
819     rv = MakeContentObject(aNodeType, nodeInfo, aForm, aWebShell,
820                            aResult, &content, !!mInsideNoXXXTag);
                                                  ^^^
...
827   return rv;
828 }

Comment 1

[harishd]

That change was made by Johnny. 

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&fi
le=nsHTMLContentSink.cpp&root=/cvsroot&subdir=mozilla/content/html/document/src&
command=DIFF_FRAMESET&rev1=3.387&rev2=3.388

Reassigning to jst.

Comment 2

[Fabian Guisset]

Talked with jst about this:
This is as designed. Since PRBool in c++ is basically the same as PRInt32, we
need to make sure the value passed to MakeContentObject() is 0 or 1. the "!!" is
really a double "not" operator. Let's say the value of mInsideNoXXXTag is 5 (for
whatever reason), then !mInsideNoXXXTag will be 0, and !!mInsideNoXXXTag will be
1. If mInsideNoXXXTag is 0, !!mInsideNoXXXTag will be 0 and everything is fine.
Marking invalid.

Comment 3

[rbs]

In that case, it would be better to add a comment in the code (or to simply
use a temporary variable, e.g., state = position == mInsideNoXXXTag, or
something like that, since optimization shouldn't be the issue there).
Otherwise, someone reading the code can easily be alarmed in thinking that,
after a long day, the finger stays a bit longer than necessary on the keyboard
there.

Comment 4

[Chris Waterson]

rbs is right, this _so_ looks like a typo. How about, |mInsideTag != 0| (Man, 
I'm not even sure I got that right.) Re-opening; please clean this up! 
Reassigning to harishd, because I know jst is busy landing XPC-DOM.

Comment 5

[Chris Waterson]

-> harishd

Comment 6

[Johnny Stenback (:jst)]

Geez, what's wrong with you, Chris :-) !! is used in a bunch of places in the
code, this is not the first and only one, there's nothing wrong with !!, get
used to it! ;)

Comment 7

[Johnny Stenback (:jst)]

I suggest WONTFIX, btw.

Comment 8

[Brendan Eich [:brendan]]

!! is idiosyncratic, not quite a true idiom: the English idiom "kick the bucket"
means "die", but you can't tell from the denotations or connotations of the
constituent words.  !!, OTOH, can be parsed if you write the ! boolean function
map and run through it twice.  But it does make some peoples' heads hurt!

Ok, so it's oddball, but it works and some people like it.  I say module owner
gets to decide.  Is this file owned by one person?  Can't we all just get along?
WONTFIX is plausible if not, INVALID if so.

/be

Comment 9

[rbs]

The fact that mInsideNoXXXTag just looks like a boolean itself makes things more
obscure. I would hope the module owner to avoid this controversial practice that
doesn't help readability since this isn't a code obfuscation contest.

Comment 10

[Brendan Eich [:brendan]]

rbs: good point, if it were mNoXXXTagLevel or some such, the !! would be more
easily understood.  Even waterson's != 0, which I would have used over !!, is
not so clear, given the name.  I think you've pinned down the real problem here,
if there is one.  My 2 cents.

/be

Comment 11

[Johnny Stenback (:jst)]

There's alot of history in this this code, we didn't make up the name
mInsideNoXXXTag initially as a number, my guess is that it was a boolean to
start with and later on the type and the meaning was changed w/o changing the
name of the variable. If someone wants to come up with a patch to rename the
variable so that the name reflects what it's used for I'd be glad to take it,
Harish?

Comment 12

[harishd]

Deferring till m1.0

Comment 13

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 14

[harishd]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 15

[Peter Van der Beken [:peterv]]

Nothing wrong with !! imho. There's some instances in Transformiix code too, so
it's spreading  ;-).

Comment 16

[rbs]

Viruses spread too, and fast. But that doesn't mean they are welcome.

Excerpt from bug 183415:
222         if (!iid.equals(nsIProxyAutoConfig) &&
223             !!iid.equals(Components.interfaces.nsIStreamListener) &&
224             !iid.equals(Components.interfaces.nsISupports)) {

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Assigning to default owner & QA. This might be a "good first bug" to learn about the HTML content sink, but I'm not sure if it qualifies.

Comment 18

[Henri Sivonen (:hsivonen)]

Per comment 7 and comment 15 and considering that this code is going away anyway, I'm taking the liberty to mark this WONTFIX.