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

RESOLVED WONTFIX

Status

()

Core
HTML: Parser
--
minor
RESOLVED WONTFIX
17 years ago
8 years ago

People

(Reporter: rbs, Unassigned)

Tracking

({helpwanted})

Trunk
Future
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WONTFIX?], URL)

(Reporter)

Description

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

17 years ago
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.
Assignee: harishd → jst

Comment 2

17 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → INVALID
(Reporter)

Comment 3

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

17 years ago
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Comment 5

17 years ago
-> harishd
Assignee: jst → harishd
Severity: normal → minor
Status: REOPENED → NEW
Summary: Critical typo: !!mInsideNoXXXTag → fancy use of double-negation (!!mInsideNoXXXTag) looks like typo

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
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! ;)
I suggest WONTFIX, btw.
!! 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
(Reporter)

Comment 9

17 years ago
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.
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
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

17 years ago
Deferring till m1.0
Target Milestone: mozilla0.9.2 → mozilla1.0

Updated

17 years ago
QA Contact: bsharma → moied

Comment 13

17 years ago
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)
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 14

16 years ago
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. 
Target Milestone: mozilla1.0.1 → Future

Updated

15 years ago
Blocks: 183415
Nothing wrong with !! imho. There's some instances in Transformiix code too, so
it's spreading  ;-).
(Reporter)

Comment 16

15 years ago
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)) {
No longer blocks: 183415
Depends on: 183415
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.
Assignee: harishd → mrbkap
Status: ASSIGNED → NEW
Keywords: helpwanted
QA Contact: moied → parser

Updated

9 years ago
Assignee: mrbkap → nobody
Per comment 7 and comment 15 and considering that this code is going away anyway, I'm taking the liberty to mark this WONTFIX.
Status: NEW → RESOLVED
Last Resolved: 17 years ago8 years ago
Resolution: --- → WONTFIX
Whiteboard: [WONTFIX?]
You need to log in before you can comment on or make changes to this bug.