Bug 518633 added some C++-style comments to js/src/jsutil.h, which causes 22 copies of this build warning (for each C file that includes this header): { > In file included from /scratch/work/builds/mozilla-central/mozilla-central.09-09-18.12-43/mozilla/js/jsd/jsd.h:78, > from /scratch/work/builds/mozilla-central/mozilla-central.09-09-18.12-43/mozilla/js/jsd/jsd_high.c:42: > ../../dist/include/jsutil.h:268:9: warning: C++ style comments are not allowed in ISO C90 > ../../dist/include/jsutil.h:268:9: warning: (this will be reported only once per input file) }

This fixes the problem. This also adds one space to the indentation of the second comment's chunk, too -- that chunk's indentation was previously off, based on the surrounding code.

I don't understand this warning. The comments are inside a C++ class. This looks like a compiler bug to me.

Mm, good point -- that chunk of the header is supposed to be ignored by C-only files that #include it, since it's wrapped with "#ifdef __cplusplus"... FWIW, I get this build warning using gcc-4.3.3 on one machine, and 4.3.4 on another machine. (The first runs Ubuntu 9.04, the second Ubuntu 9.10 beta.) I've verified that the command that triggers the warning is a "gcc" command, rather than "g++", too -- so this isn't just a case of a C file accidentally being compiled with the C++ compiler. I'm rebuilding with gcc-4.4.1 now on the Ubuntu 9.10 box, to see if that shows the warning as well.

Apparently this is a known bug in gcc: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39603 which is duped to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=2881 which is untouched since 2001 :( Given that there doesn't seem to be any activity on fixing this GCC bug, I'd still argue for applying my patch, to ameliorate warning-spam. (22 copies of a 4-line build warning = 88 lines of useless warnings...)

(In reply to comment #4) > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=2881 > which is untouched since 2001 :( oh, sorry -- I didn't really read that bug report before. The last comment there says that they don't think it's a GCC bug. Apparently comments are supposed to be removed **before** preprocessing directives are evaluated. So, C compilers will still see this comment (and either remove it or not, depending on how smart they are) before excising its block of __cplusplus code. Anyway, regardless of whether the warning is indicative of a GCC bug or not, it's trivial to work around the warning, so I assert that we should. :)

Comment on attachment 407071 [details] [diff] [review] fix: switch two comments from C++ style "//" to C style "/* */" > ~JSGuardObjectNotificationReceiver() { >- // Assert that the guard object was not used as a temporary. >- // (Note that this assert might also fire if Init is not called >- // because the guard object's implementation is not using the >- // above macros correctly.) >+ /* Assert that the guard object was not used as a temporary. >+ * (Note that this assert might also fire if Init is not called >+ * because the guard object's implementation is not using the >+ * above macros correctly.) */ > JS_ASSERT(mStatementDone); SpiderMonkey style for comments is to always use C-style and multiline comments should have the form: /* * line 1 * line 2 */ I will r+ the patch with that fixed.

Here's the fixed patch, per comment 6.

I don't mind the comment fix, but I would suggest completely disabling the code if ndef __cplusplus.

(In reply to comment #8) I don't think I understand your suggestion -- can you elaborate?

You are compiling C++ code with the C compiler. We shouldn't do that. There is a define that is only set when you are compiling with a C++ compiler (I think its __cplusplus). We can hide the class behind that (including the comment, whatever style we use for it).

Hiding the comment doesn't protect it from being seen by C compilers, because they strip comments before evaluating #ifdefs -- see comment 5!

(In reply to comment #10) > __cplusplus). We can hide the class behind that (including the comment, We already do hide the class behind "#ifdef __cplusplus", as noted in comment 3.

Ah ok sorry, didn't see that. Never mind then.

Pushed to tracemonkey: http://hg.mozilla.org/tracemonkey/rev/75904c49925a