780965 - jsutil.h:345:5: warning: C++ style comments are not allowed in ISO C90 [enabled by default] (also jsapi.h)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

Newish build warnings, for C++ comments introduced in headers that are included by .c files (from bug 777190 and bug 742188):

In file included from ../../../../mozilla/js/src/jsapi.h:22:0,
                 from ../../../../mozilla/js/src/jsapi-tests/valueABI.c:8:
../../../../mozilla/js/src/jsutil.h:345:5: warning: C++ style comments are not allowed in ISO C90 [enabled by default]
../../../../mozilla/js/src/jsutil.h:345:5: warning: (this will be reported only once per input file) [enabled by default]
In file included from ../../../../mozilla/js/src/jsapi-tests/valueABI.c:8:0:
../../../../mozilla/js/src/jsapi.h:2871:9: warning: C++ style comments are not allowed in ISO C90 [enabled by default]
../../../../mozilla/js/src/jsapi.h:2871:9: warning: (this will be reported only once per input file) [enabled by default]

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Here's the fix.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 649759 [details] [diff] [review]
fix

>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>--- a/js/src/jsapi.h
>+++ b/js/src/jsapi.h
>@@ -2863,17 +2863,19 @@ ToUint64(JSContext *cx, const js::Value 
> {
>     AssertArgumentsAreSane(cx, v);
>     {
>         SkipRoot skip(cx, &v);
>         MaybeCheckStackRoots(cx);
>     }
> 
>     if (v.isInt32()) {
>-        // Account for sign extension of negatives into the longer 64bit space.
>+        /*
>+         * Account for sign extension of negatives into the longer 64bit space.
>+         */

(BTW, I made this one a 3-line C-style comment, because as a one-liner, the trailing " */" would push it over 80 characters.)

Comment 3

[:Benjamin Peterson]

Comment on attachment 649759 [details] [diff] [review]
fix

Review of attachment 649759 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +2868,5 @@
>      }
>  
>      if (v.isInt32()) {
> +        /*
> +         * Account for sign extension of negatives into the longer 64bit space.

Can this be on one line?

Comment 4

[:Benjamin Peterson]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> (BTW, I made this one a 3-line C-style comment, because as a one-liner, the
> trailing " */" would push it over 80 characters.)

Sorry, I didn't see this comment before making mine.

We wrap JS code at 100.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 649759 [details] [diff] [review]
fix

Thanks!

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Benjamin Peterson from comment #4)
> Sorry, I didn't see this comment before making mine.
> 
> We wrap JS code at 100.

ah, ok -- that's easier then.

(I skimmed the contextual code and saw some wrapped-at-80 chunks, which was why I assumed 80)

I'll condense that to one line before pushing.  Thanks!

Comment 7

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4b4a45e4cf

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f4b4a45e4cf

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Benjamin Peterson [:benjamin] from comment #4)
> We wrap JS code at 100.

This isn't quite true, two ways.  First, we wrap at 99 characters because some editors are deficient and in the 100ch case show a line-wraps character rather than that 100th character.  Second, we wrap comments at 79ch because it's more pleasant to read prose that's slightly narrower.

As for why there's some wrapped-at-80 chunks, it's because 79 was the limit historically, and 100ch was only incrementally adopted, so older code, or code not touched enough, recently, may not be 100ch-wrapped.

I might remember to put this change in an mq at some point, once I tunnel out of my bugmail backlog, but ideally someone else will just push an r=lumpy patch before then, and before I forget about this.  :-)