Last Comment Bug 780965 - jsutil.h:345:5: warning: C++ style comments are not allowed in ISO C90 [enabled by default] (also jsapi.h)
: jsutil.h:345:5: warning: C++ style comments are not allowed in ISO C90 [enabl...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Daniel Holbert [:dholbert] (largely AFK until June 28)
:
Mentors:
Depends on:
Blocks: buildwarning 742188 777190
  Show dependency treegraph
 
Reported: 2012-08-07 12:33 PDT by Daniel Holbert [:dholbert] (largely AFK until June 28)
Modified: 2012-08-27 17:43 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (2.25 KB, patch)
2012-08-07 12:43 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
jorendorff: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-08-07 12:33:42 PDT
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] (largely AFK until June 28) 2012-08-07 12:43:11 PDT
Created attachment 649759 [details] [diff] [review]
fix

Here's the fix.
Comment 2 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-08-07 12:44:33 PDT
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 2012-08-07 13:30:53 PDT
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 2012-08-07 13:31:39 PDT
(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] 2012-08-07 13:49:50 PDT
Comment on attachment 649759 [details] [diff] [review]
fix

Thanks!
Comment 6 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-08-07 13:55:56 PDT
(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] (largely AFK until June 28) 2012-08-07 14:34:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4b4a45e4cf
Comment 8 Ed Morley [:emorley] 2012-08-08 09:32:53 PDT
https://hg.mozilla.org/mozilla-central/rev/1f4b4a45e4cf
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2012-08-27 17:43:50 PDT
(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.  :-)

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