621201 - JS_STATIC_ASSERT modernization (also provides HP-UX/aCC fix)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Paprocki]

Attachment: 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]]

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]]

Er, s/AIX/<whatever list of esoteric platforms is desirable>/

Comment 3

[Andrew Paprocki]

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]]

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]

-std=c++0x defines __GXX_EXPERIMENTAL_CXX0X__. It doesn't accept it otherwise.

Comment 6

[Brendan Eich [:brendan]]

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]

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]

Attachment: Use C++0x static_assert() for JS_STATIC_ASSERT when possible

Removed erroneous #include for hpux in the previous patch.

Comment 9

[Igor Bukanov]

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]

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]

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]

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]

Attachment: 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]

Attachment: Add static_assert check to configure, update jsutil.h to use it.

Updated configure.in test logic to be more sane.

Comment 15

[(not currently active) Ted Mielczarek]

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]

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]

(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]

(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]

(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]]

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]]

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]

Looks great, you're the man!