CHECK_VALID_NODEINFO needs to be checked

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
The macro CHECK_VALID_NODEINFO has a few problems I can see.

- The first thing I noticed is that after the conversion to nullptr, we get the following warning message in GCC:
  nsNodeInfoManager.cpp:259: warning: NULL used in arithmetic
This is because the macro does (_extraName != nullptr) when it should do !!_extraName.
- Looking at the macro, I noticed that the argument _namespaceID is unused, presumably because aNamespaceID later in the macro should be _namespaceID. Fortunately it doesn't really matter because every _namespaceID argument is aNamespaceID.
- This should probably be converted to an inline function, as that would avoid the previous problem.
(Assignee)

Comment 1

5 years ago
Created attachment 651806 [details] [diff] [review]
de-macro and fix

Unfortunately, this requires including a few more headers, but maybe that's not a big deal.  I could also move the definition to the cpp and just lose the inlining, or just leave it as a sketchy macro.
(Assignee)

Comment 2

5 years ago
Created attachment 651809 [details] [diff] [review]
de-macro and fix

I forgot to fix the (foo != nullptr) thing, but apparently de-macroizing it shuts up the warning.  Awesome.  Fixed it now anyways.
Attachment #651806 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Comment on attachment 651809 [details] [diff] [review]
de-macro and fix

I can fix this another way if you prefer.
Attachment #651809 - Flags: review?(bugs)

Updated

5 years ago
Attachment #651809 - Flags: review?(bugs) → review+
(Assignee)

Comment 4

5 years ago
Thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/75f5849d6495
https://hg.mozilla.org/mozilla-central/rev/75f5849d6495
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.