Last Comment Bug 621201 - JS_STATIC_ASSERT modernization (also provides HP-UX/aCC fix)
: JS_STATIC_ASSERT modernization (also provides HP-UX/aCC fix)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-23 12:28 PST by Andrew Paprocki
Modified: 2012-01-22 05:25 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use C++0x static_assert() for JS_STATIC_ASSERT when possible (3.01 KB, patch)
2010-12-23 12:28 PST, Andrew Paprocki
igor: review+
Details | Diff | Splinter Review
Use C++0x static_assert() for JS_STATIC_ASSERT when possible (2.87 KB, patch)
2011-04-13 09:05 PDT, Andrew Paprocki
igor: review+
Details | Diff | Splinter Review
Add static_assert check to configure, update jsutil.h to use it. (4.46 KB, patch)
2011-07-06 05:28 PDT, Andrew Paprocki
no flags Details | Diff | Splinter Review
Add static_assert check to configure, update jsutil.h to use it. (4.55 KB, patch)
2011-07-06 05:38 PDT, Andrew Paprocki
no flags Details | Diff | Splinter Review

Description Andrew Paprocki 2010-12-23 12:28:17 PST
Created attachment 499554 [details] [diff] [review]
Use C++0x static_assert() for JS_STATIC_ASSERT when possible

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
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2010-12-23 12:37:22 PST
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 Jeff Walden [:Waldo] (remove +bmo to email) 2010-12-23 12:40:52 PST
Er, s/AIX/<whatever list of esoteric platforms is desirable>/
Comment 3 Andrew Paprocki 2010-12-23 12:45:49 PST
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 Jeff Walden [:Waldo] (remove +bmo to email) 2010-12-23 13:02:33 PST
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?
Comment 5 Andrew Paprocki 2010-12-23 13:04:37 PST
-std=c++0x defines __GXX_EXPERIMENTAL_CXX0X__. It doesn't accept it otherwise.
Comment 6 Brendan Eich [:brendan] 2010-12-27 15:23:28 PST
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
Comment 7 Igor Bukanov 2010-12-28 13:41:42 PST
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.
Comment 8 Andrew Paprocki 2011-04-13 09:05:53 PDT
Created attachment 525688 [details] [diff] [review]
Use C++0x static_assert() for JS_STATIC_ASSERT when possible

Removed erroneous #include for hpux in the previous patch.
Comment 9 Igor Bukanov 2011-04-13 12:45:55 PDT
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__
Comment 10 Paul Biggar 2011-04-13 16:59:31 PDT
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?
Comment 11 Andrew Paprocki 2011-04-13 17:07:11 PDT
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 Paul Biggar 2011-04-13 17:50:09 PDT
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.
Comment 13 Andrew Paprocki 2011-07-06 05:28:26 PDT
Created attachment 544206 [details] [diff] [review]
Add static_assert check to configure, update jsutil.h to use it.

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.
Comment 14 Andrew Paprocki 2011-07-06 05:38:46 PDT
Created attachment 544210 [details] [diff] [review]
Add static_assert check to configure, update jsutil.h to use it.

Updated configure.in test logic to be more sane.
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-07-06 07:00:46 PDT
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.
Comment 16 Andrew Paprocki 2011-07-06 07:26:46 PDT
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 Igor Bukanov 2011-07-06 09:35:17 PDT
(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 Paul Biggar 2011-07-06 12:50:39 PDT
(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 Igor Bukanov 2011-07-08 10:33:55 PDT
(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 Daniel Holbert [:dholbert] 2011-07-13 12:31:55 PDT
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.)
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-21 22:07:22 PST
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/ )
Comment 22 Andrew Paprocki 2011-12-22 06:27:16 PST
Looks great, you're the man!

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