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

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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]
(Assignee)

Comment 1

5 years ago
Created attachment 649759 [details] [diff] [review]
fix

Here's the fix.
Assignee: general → dholbert
Status: NEW → ASSIGNED
Attachment #649759 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 2

5 years ago
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

5 years ago
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

5 years ago
(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 on attachment 649759 [details] [diff] [review]
fix

Thanks!
Attachment #649759 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 6

5 years ago
(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!
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4b4a45e4cf
Target Milestone: --- → mozilla17

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1f4b4a45e4cf
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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.  :-)
You need to log in before you can comment on or make changes to this bug.