Last Comment Bug 782677 - CHECK_VALID_NODEINFO needs to be checked
: CHECK_VALID_NODEINFO needs to be checked
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Andrew McCreight [:mccr8]
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2012-08-14 09:26 PDT by Andrew McCreight [:mccr8]
Modified: 2012-08-16 17:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

de-macro and fix (8.83 KB, patch)
2012-08-14 10:04 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
de-macro and fix (8.82 KB, patch)
2012-08-14 10:09 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-08-14 09:26:37 PDT
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.
Comment 1 Andrew McCreight [:mccr8] 2012-08-14 10:04:56 PDT
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.
Comment 2 Andrew McCreight [:mccr8] 2012-08-14 10:09:55 PDT
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.
Comment 3 Andrew McCreight [:mccr8] 2012-08-14 10:14:09 PDT
Comment on attachment 651809 [details] [diff] [review]
de-macro and fix

I can fix this another way if you prefer.
Comment 4 Andrew McCreight [:mccr8] 2012-08-16 08:08:41 PDT
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-08-16 17:57:46 PDT

Note You need to log in before you can comment on or make changes to this bug.