573895 - nsGenericHTMLElement.h(1319) : warning C4003: not enough actual parameters for macro 'NS_DECLARE_NS_NEW_HTML_ELEMENT'

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

RyanVM pointed out to me that we now get many instances of this warning on Windows:

{
e:\builds\moz2_slave\mozilla-central-win32-debug\build\content\html\content\src\nsGenericHTMLElement.h(1319) : warning C4003: not enough actual parameters for macro 'NS_DECLARE_NS_NEW_HTML_ELEMENT'
}

I get 101 instances of this exact warning one for each in the build log linked in the URL field. (one instance per .cpp file that includes nsGenericHTMLElement.h, I think)

This traces back to these lines:

> 1306 #define NS_DECLARE_NS_NEW_HTML_ELEMENT(_elementName)              \
> 1307 nsGenericHTMLElement*                                             \
> 1308 NS_NewHTML##_elementName##Element(nsINodeInfo *aNodeInfo,         \
> 1309                                   PRUint32 aFromParser = 0);
[...]
> 1319 NS_DECLARE_NS_NEW_HTML_ELEMENT() // HTMLElement

(with the final line being new a few days ago, from Bug 562008)

So what's going on is: the macro wants an argument, and line 1319 passes the empty string for the argument (to declare the method "NS_NewHTMLElement").  But MSVC apparently worries that we're forgetting that there's supposed to be an argument.

From some quick Googling, it doesn't seem that there's an easy way around this -- I think we might just need to wrap line 1319 with a #pragma to disable this MSVC warning.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> I get 101 instances of this exact warning one for each in the build log linked
> in the URL field. (one instance per .cpp file that includes
> nsGenericHTMLElement.h, I think)

(sorry, ignore the text "one for each" in the first line there)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix: disable warning

This fix should disable the warning for this line that triggers it.  Running it through TryServer right now to make sure.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v2: fix call to NS_IMPL_NS_NEW_HTML_ELEMENT in nsHTMLElement.cpp

I noticed this warning actually also gets spammed for a line in nsHTMLElement.cpp, too.  This patch version applies the fix there, too.  (I think this is the only other place where we spam MSVC warning C4003.)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 453510 [details] [diff] [review]
fix v2: fix call to NS_IMPL_NS_NEW_HTML_ELEMENT in nsHTMLElement.cpp

First patch version was successful at fixing the failures for NS_DECLARE_NS_NEW_HTML_ELEMENT on tryserver[1].  (The only instance of warning C4003 in that tryserver log is the one warning for nsHTMLElement.cpp, which I caught in comment 3 and fixed in fix v2)

Requesting review on fix v2.

[1] http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1277337211.1277350068.7989.gz

Comment 5

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/0030881d9ad1

Comment 6

[Daniel Holbert [:dholbert]]

Apparently we can get this warning on GCC too -- mats reports via IRC:
> gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5) says:
> nsGenericHTMLElement.h:1324:32: error: invoking macro NS_DECLARE_NS_NEW_HTML_ELEMENT argument 1: empty macro arguments are undefined in ISO C90 and ISO C++98

I have the same GCC version as he does, and yet I'm not seeing that warning.  (Tinderbox doesn't hit it, either.)  investigating to figure out what's going on...

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> I have the same GCC version as he does

D'oh -- I have the same GCC version *installed*, but I forgot that I have my CXX bash variable set to actually *use* an earlier version (4.3.3).  I do hit this warning in GCC when I use version 4.4.3.

Comment 8

[Daniel Holbert [:dholbert]]

So this warning is included under the header of "-pedantic" -- it doesn't have a specific warning identifier (there's no e.g. -Wno-empty-macros)

I tried adding
> #pragma GCC diagnostic ignored "-pedantic"
to nsGenericHTMLElement.h per http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html but it didn't seem to have an effect. (I suspect that only works for "-Wfoo" type warnings?)

Comment 9

[Mats Palmgren (inactive)]

I couldn't find a solution either, but if it's just
NS_DECLARE_NS_NEW_HTML_ELEMENT() we could remove it
and declare NS_NewHTMLElement directly instead.

Comment 10

[Mats Palmgren (inactive)]

Fwiw, I looked at the gcc patch[1] and realized there is a solution:
#pragma GCC system_header
NS_DECLARE_NS_NEW_HTML_ELEMENT() // HTMLElement

It looks like it mutes pedantic warnings for the rest of that header file. [2]
(note that it could have other unknown effects though)

[1]  http://gcc.gnu.org/viewcvs/trunk/libcpp/macro.c?r1=137414&r2=137413&pathrev=137414
[2]  http://everything2.com/title/%2523pragma

Comment 11

[Daniel Holbert [:dholbert]]

Thanks Mats! Good to know that there's *some* way to suppress that warning. :)

I think I prefer comment 9 (expanding the macro in this instance) over comment 10, though.  AFAICT, there's no way to flag just a single line with "#pragma system_header", and I think flagging the whole rest of the file (as we'd end up doing) would be too large of a hammer for this nail.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: better fix: expand 0-argument NS_xxx_NS_NEW_HTML_ELEMENT invocations

(In reply to comment #11)
> I think I prefer comment 9 (expanding the macro in this instance)

This new patch expands the no-argument two NS_xxx_NS_NEW_HTML_ELEMENT() macro-invocations, to work around the GCC 4.4 version of this warning. (sadly there's not really a good targeted way to suppress the warning in GCC, as discussed in the last few comments)

Comment 13

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/c3f14ba8c825