Closed
Bug 621201
Opened 13 years ago
Closed 12 years ago
JS_STATIC_ASSERT modernization (also provides HP-UX/aCC fix)
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: andrew, Unassigned)
Details
Attachments
(2 files, 2 obsolete files)
2.87 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
4.55 KB,
patch
|
Details | Diff | Splinter Review |
The current definition of JS_STATIC_ASSERT is an array-based static assertion utilizing an extern function declaration. This does not work well in the aCC compiler because it complains about linkage issues. To fix this, the real C++0x static_assert() could be used when available, as aCC supports this in C++0x mode (-Ax). Building Spidermonkey on HP-UX would then be possible with the -Ax flag. This patch will use C++0x static_assert() prior to the older definitions on these compilers: GNU G++ >= 4.3 w/ -std=c++0x IBM xlC >= 11 w/ -qlanglvl=extended0x HP aCC >= 6.25 w/ -Ax MS VC >= 2010
Attachment #499554 -
Flags: review?(brendan)
Comment 1•13 years ago
|
||
Comment on attachment 499554 [details] [diff] [review] Use C++0x static_assert() for JS_STATIC_ASSERT when possible I don't think that, right now, we should take a patch to change how JS_STATIC_ASSERT works on supported platforms. Narrowly targeted for AIX, sure. But not this way, not right now. After 4.0 is out and/or after we've fixed the 4.0 JS blockers and branched, let's consider it. Strawman thought for the future: #define a static_assert(cond, msg) macro which expands to what JS_STATIC_ASSERT expands to now, on platforms that don't have C++0x static-assert support? Seems better to use the standard thing than to craft our own alternative, if the two are compatible (which they are, enough, here).
Comment 2•13 years ago
|
||
Er, s/AIX/<whatever list of esoteric platforms is desirable>/
Reporter | ||
Comment 3•13 years ago
|
||
If there is consensus on that, I could modify the patch so simply do the detection for AIX and HP-UX and leave GCC/MSVC alone. Just seemed a shame to not take advantage of it.
Comment 4•13 years ago
|
||
I agree it's a shame in at least some sense. But now's not the time to make larger changes than necessary without some benefit to them. I'm surprised gcc accepts static_assert without -std=c++0x. Or is __GXX_EXPERIMENTAL_CXX0X__ how you detect when that's been specified?
Reporter | ||
Comment 5•13 years ago
|
||
-std=c++0x defines __GXX_EXPERIMENTAL_CXX0X__. It doesn't accept it otherwise.
Comment 6•13 years ago
|
||
Comment on attachment 499554 [details] [diff] [review] Use C++0x static_assert() for JS_STATIC_ASSERT when possible Igor is master of JS_STATIC_ASSERT. /be
Attachment #499554 -
Flags: review?(brendan) → review?(igor)
Comment 7•13 years ago
|
||
Comment on attachment 499554 [details] [diff] [review] Use C++0x static_assert() for JS_STATIC_ASSERT when possible >+#ifdef __hpux >+#include <string.h> /* memset() */ >+#endif It is unclear what exactly memset means here. The comments must write briefly the reason for the header inclusion. r+ with that fixed.
Attachment #499554 -
Flags: review?(igor) → review+
Reporter | ||
Comment 8•13 years ago
|
||
Removed erroneous #include for hpux in the previous patch.
Attachment #499554 -
Attachment is obsolete: true
Attachment #525688 -
Flags: review?
Reporter | ||
Updated•13 years ago
|
Attachment #525688 -
Flags: review? → review?(igor)
Comment 9•13 years ago
|
||
Comment on attachment 525688 [details] [diff] [review] Use C++0x static_assert() for JS_STATIC_ASSERT when possible >diff -r d78f9cb65e91 js/src/jsutil.h >-#ifdef __SUNPRO_CC >+#ifdef __cplusplus >+/* GCC 4.3 has support with -std=c++0x */ >+#if defined(__GNUC__) && defined(__GXX_EXPERIMENTAL_CXX0X__) Nit: move comments after the if here and below in in the compiler-specific ifs. Nit: add an extra blank before # to account for ifdef __cplusplus nesting >+#if defined(JS_CXX0x_STATIC_ASSERT) >+# define JS_STATIC_ASSERT(cond) \ >+ static_assert(cond, #cond) Nit: do not put static_assert on own line. >+#elif defined(__COUNTER__) Nit: comment what is __COUNTER__
Attachment #525688 -
Flags: review?(igor) → review+
Comment 10•13 years ago
|
||
I think we should move away from doing this sort of feature checking in code, and move it into configure, where it belongs. But perhaps that should be a different bug?
Reporter | ||
Comment 11•13 years ago
|
||
I would have done it there but I don't know the preferred way / nits (naming? location?) for adding something to configure and haven't done it before in sm. I just needed to get this compiling on my systems. So my preference would be to land this and then have another bug to move it out completely to configure, but it is up to you guys.
Comment 12•13 years ago
|
||
It sounds best to land this now, and do the rest in a follow-on bug. The follow-on bug should look at js/src/configure.in. You can ask for feedback and reviews from ted and jimb, and I think I know enough to help too. Please CC me on the bug.
Reporter | ||
Updated•13 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 13•13 years ago
|
||
Guys, I've added a new patch here which adds the static_assert detection to configure.in as requested above. (Just added it to this bug since the original patch was never checked in.) Please review this one and if it looks good, then I'll obsolete the old one.
Attachment #544206 -
Flags: review?
Reporter | ||
Updated•13 years ago
|
Attachment #544206 -
Flags: review? → review?(igor)
Reporter | ||
Comment 14•13 years ago
|
||
Updated configure.in test logic to be more sane.
Attachment #544206 -
Attachment is obsolete: true
Attachment #544210 -
Flags: review?(igor)
Attachment #544206 -
Flags: review?(igor)
Comment 15•13 years ago
|
||
Personally, I don't think this checking would be any cleaner in configure than it is in this header file. I realize that the "autoconf way" is to do all such checks in configure, but if you can easily test the conditions you need with the preprocessor then I'm not opposed to it. You've got all the complexity contained in a single header file, which feels right.
Reporter | ||
Comment 16•13 years ago
|
||
I'm all for leaving it in the header, as you get the benefit of seeing which compilers explicitly support it in the comments / code. Either way, both patches are here.
Comment 17•13 years ago
|
||
(In reply to comment #16) > I'm all for leaving it in the header, as you get the benefit of seeing which > compilers explicitly support it in the comments / code. Yes, lets leave the assert in the header.
Comment 18•13 years ago
|
||
(In reply to comment #15) > Personally, I don't think this checking would be any cleaner in configure > than it is in this header file. I realize that the "autoconf way" is to do > all such checks in configure, but if you can easily test the conditions you > need with the preprocessor then I'm not opposed to it. You've got all the > complexity contained in a single header file, which feels right. I don't understand this. The "autoconf way" is feature-detection, which is better in every way than "enumerate supported platforms". What am I missing?
Comment 19•13 years ago
|
||
(In reply to comment #14) > Created attachment 544210 [details] [diff] [review] [review] > Add static_assert check to configure, update jsutil.h to use it. Would the try server show any failure if we define the static assert as extern char js_static_assert[(cond) ? 1 : -1] on platforms where static_assert is not available? If the try run would be green, then I would prefer a configure patch with a minimal fallback like: #if HAVE_STATIC_ASSERT # define JS_STATIC_ASSERT(cond) static_assert(cond, #cond) #else # define JS_STATIC_ASSERT(cond) extern char js_static_assert[(cond) ? 1 : -1] #endif I.e. if we can avoid any platform selection in the header delegating all job to configure, then this would be a clear win.
Comment 20•13 years ago
|
||
Based on comment 13, it sounds like this isn't checkin-needed yet (as the only currently-r+'d patch is potentially going to be obsoleted). Removing "checkin-needed" flag -- add it back once there's a reviewed patch that is ready to be landed. (Apologies if I'm misunderstanding anything.)
Keywords: checkin-needed
Comment 21•12 years ago
|
||
Andrew, I think I fixed everything you wanted fixed in the commits I just made for bug 712129 -- I made an effort to include the portions of the patches here that seemed relevant. Do those commits fix this bug? If so, please close it; if not, the new location for the static-assertion implementation is now mfbt/Assertions.h. (It's being shared between SpiderMonkey code and browser code, at long last. \o/ )
Reporter | ||
Comment 22•12 years ago
|
||
Looks great, you're the man!
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #544210 -
Flags: review?(igor)
You need to log in
before you can comment on or make changes to this bug.
Description
•