Closed Bug 381236 Opened 13 years ago Closed 13 years ago

JS_STATIC_ASSERT's use of the line number breaks the build

Categories

(Core :: JavaScript Engine, defect, major)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: igor)

References

Details

Attachments

(1 file, 1 obsolete file)

In file included from ../../../mozilla/js/src/jsscan.h:48,
                 from ../../../mozilla/js/src/jsatom.c:57:
../../../mozilla/js/src/jsopcode.h:214: error: redefinition of `js_static_assert_line_215'
../../../mozilla/js/src/jsgc.h:215: error: `js_static_assert_line_215' previously declared here

I have three lines of comment added at the top of jsgc.h, with the result that 

  JS_STATIC_ASSERT(JSTRACE_STRING == 2);

in that header ends up on line 215.  In jsopcode.h, lines 214 and 215 are:

  JS_STATIC_ASSERT(sizeof(jsatomid) * JS_BITS_PER_BYTE >=
                   ATOM_INDEX_LIMIT_LOG2 + 1);

I think the upshot is that these asserts should not be going into header files, unless you're sure the headers will never be included in the same compilation unit.
I can't think of a solution.  Too bad we can't use __FILE__ to
disambiguate the typedef.
The only solution I see is to not put these into headers.
Or bz can add extra blank lines to one of the files as a workaround until we switch from the static-assertion-macro hack to using the equivalent C++ language/compiler feature.
I did that.  That'll work until the next time when someone changes those files, when my build might break again.  Or maybe it'll be the debug tinderboxen next time, depending on how the files get changed, of course.  ;)
Attached patch Fix (obsolete) — Splinter Review
This should fix the issue: the idea is to expand static_assert into an extern declaration that can be repeated. r=+ as this is a part of the patch for bug 379758 that already got r=+ from Brendan.
Assignee: general → igor
Status: NEW → ASSIGNED
Attachment #265578 - Flags: review+
Comment on attachment 265578 [details] [diff] [review]
Fix

>+    extern void js_static_assert_line_##line(int arg[(condition) ? 1 : -1])

Does this work?  If such an extern declaration can be
repeated, you should be able to omit "_line_##line"
from the identifier's name.
Attached patch fix v2Splinter Review
The new version removes the __LINE__ from the macro as extern can be used arbitrary number of times. This works with GCC, can anybody check the patch with MSVC?
Attachment #265578 - Attachment is obsolete: true
Attachment #265580 - Flags: review?(brendan)
If we don't use __LINE__ and ##, we can simplify JS_STATIC_ASSERT
further by eliminating the intermediate macro JS_STATIC_ASSERT_IMPL.

Can we use an extern declaration of just the array?

#define JS_STATIC_ASSERT(condition) \
    extern int js_static_assert[(condition) ? 1 : -1]
(In reply to comment #7)
> Created an attachment (id=265580) [details]
> fix v2
> 
> This works with GCC, can anybody check the patch with MSVC?

Applying that patch and rebuilding js/src works fine, using msvc 7.1
(In reply to comment #8)
> If we don't use __LINE__ and ##, we can simplify JS_STATIC_ASSERT
> further by eliminating the intermediate macro JS_STATIC_ASSERT_IMPL.

The last patch does exactly that and defines:

#define JS_STATIC_ASSERT(condition)                                           \
    extern void js_static_assert(int arg[(condition) ? 1 : -1])

> Can we use an extern declaration of just the array?

GCC will warn in that case about an unused declaration if one uses JS_STATIC_ASSERT inside a function. With an extern function there are no warnings.
Comment on attachment 265580 [details] [diff] [review]
fix v2

Great simplification.

/be
Attachment #265580 - Flags: review?(brendan) → review+
Comment on attachment 265580 [details] [diff] [review]
fix v2

r=wtc.  Sorry I missed that you deleted the intermediate
macro.  Good patch.
Attachment #265580 - Flags: review+
I committed the patch from comment 7 to the trunk:

Checking in jsutil.h;
/cvsroot/mozilla/js/src/jsutil.h,v  <--  jsutil.h
new revision: 3.14; previous revision: 3.13
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
The patch from bug 379758 needs extern-based JS_STATIC_ASSERT for better compatibility with older compilers as it uses the macro inside a function.
Blocks: 379758
This doesn't ever have effects that make their way through to the linker, does it?

There are some other copies of these macros in the tree (one set in configure, another in an XPCOM or NSPR header).  Should those be fixed as well?
Yes, the copy of this macro in NSPR (PR_STATIC_ASSERT) should also
be fixed.
An extern function declaration should not have any effects
that make their way to the linker.  Lots of headers declare
functions that are not referenced.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.