Closed
Bug 381236
Opened 17 years ago
Closed 17 years ago
JS_STATIC_ASSERT's use of the line number breaks the build
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: igor)
References
Details
Attachments
(1 file, 1 obsolete file)
1.38 KB,
patch
|
brendan
:
review+
wtc
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•17 years ago
|
||
I can't think of a solution. Too bad we can't use __FILE__ to disambiguate the typedef.
Reporter | ||
Comment 2•17 years ago
|
||
The only solution I see is to not put these into headers.
Comment 3•17 years ago
|
||
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.
Reporter | ||
Comment 4•17 years ago
|
||
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. ;)
Assignee | ||
Comment 5•17 years ago
|
||
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.
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
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)
Comment 8•17 years ago
|
||
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]
Comment 9•17 years ago
|
||
(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
Assignee | ||
Comment 10•17 years ago
|
||
(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 11•17 years ago
|
||
Comment on attachment 265580 [details] [diff] [review] fix v2 Great simplification. /be
Attachment #265580 -
Flags: review?(brendan) → review+
Comment 12•17 years ago
|
||
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+
Assignee | ||
Comment 13•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•17 years ago
|
||
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?
Comment 16•17 years ago
|
||
Yes, the copy of this macro in NSPR (PR_STATIC_ASSERT) should also be fixed.
Comment 17•17 years ago
|
||
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.
Updated•17 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•