460157 - Yet more macros for defining builtins

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Attachment: v1

Brendan asked me to take a swing at the redundancy in places like this, in math.cpp:

  JS_DEFINE_CALLINFO_1(static, DOUBLE, math_log_tn, DOUBLE,           1, 1)
  JS_DEFINE_CALLINFO_2(static, DOUBLE, math_max_tn, DOUBLE, DOUBLE,   1, 1)
  JS_DEFINE_CALLINFO_2(static, DOUBLE, math_pow_tn, DOUBLE, DOUBLE,   1, 1)
  JS_DEFINE_CALLINFO_1(static, DOUBLE, math_random_tn, RUNTIME,       0, 0)

  static const JSTraceableNative math_log_trcinfo =
    { math_log,    &_JS_CALLINFO(math_log_tn),      "",    "d",    INFALLIBLE };
  static const JSTraceableNative math_max_trcinfo =
    { math_max,    &_JS_CALLINFO(math_max_tn),      "",    "dd",   INFALLIBLE };
  static const JSTraceableNative math_pow_trcinfo =
    { math_pow,    &_JS_CALLINFO(math_pow_tn),      "",    "dd",   INFALLIBLE };
  static const JSTraceableNative math_random_trcinfo =
    { math_random, &_JS_CALLINFO(math_random_tn),   "R",   "",     INFALLIBLE };

With this patch, it looks like:

  JS_DEFINE_TRCINFO_1(math_log,
      (1, (static, DOUBLE, math_log_tn, DOUBLE,           1, 1)))
  JS_DEFINE_TRCINFO_1(math_max,
      (2, (static, DOUBLE, math_max_tn, DOUBLE, DOUBLE,   1, 1)))
  JS_DEFINE_TRCINFO_1(math_pow,
      (2, (static, DOUBLE, math_pow_tn, DOUBLE, DOUBLE,   1, 1)))
  JS_DEFINE_TRCINFO_1(math_random,
      (1, (static, DOUBLE, math_random_tn, RUNTIME,       0, 0)))

But if the macroorganisms in jsbuiltins.h are too disgusting to take, that's ok too.

Comment 1

[Jason Orendorff [:jorendorff]]

Those macros could be a little cleaner if we wanted to use variadic macros, but I think right now we only use them in debug builds; I wasn't sure we wanted to take the C++ compatibility hit.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Also, some people still use MSVC 7.1, see bug 453636.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 343322 [details] [diff] [review]
v1

>+#define _JS_TYPEINFO(ctype, size, pch, ach)             (ctype, size, pch, ach, INFALLIBLE)
>+#define _JS_TYPEINFO2(ctype, size, pch, ach, flags)     (ctype, size, pch, ach, flags)
>+#define _JS_TYPEINFO_CONTEXT       _JS_TYPEINFO(JSContext *,        _JS_PTR, "C", "")
>+#define _JS_TYPEINFO_RUNTIME       _JS_TYPEINFO(JSRuntime *,        _JS_PTR, "R", "")
>+#define _JS_TYPEINFO_THIS          _JS_TYPEINFO(JSObject *,         _JS_PTR, "T", "")
>+#define _JS_TYPEINFO_THIS_DOUBLE   _JS_TYPEINFO(jsdouble,           _JS_F64, "D", "")
>+#define _JS_TYPEINFO_THIS_STRING   _JS_TYPEINFO(JSString *,         _JS_PTR, "S", "")
>+#define _JS_TYPEINFO_PC            _JS_TYPEINFO(jsbytecode *,       _JS_PTR, "P", "")
>+#define _JS_TYPEINFO_JSVAL         _JS_TYPEINFO(jsval,              _JS_I32, "",  "v")
>+#define _JS_TYPEINFO_JSVAL_ERR     _JS_TYPEINFO2(jsval,             _JS_I32, --,  --, FAIL_JSVAL)
>+#define _JS_TYPEINFO_BOOL          _JS_TYPEINFO(JSBool,             _JS_I32, "",  "i")
>+#define _JS_TYPEINFO_BOOL_ERR      _JS_TYPEINFO2(int32,             _JS_I32, --,  --, FAIL_VOID)
>+#define _JS_TYPEINFO_INT32         _JS_TYPEINFO(int32,              _JS_I32, "",  "i")
>+#define _JS_TYPEINFO_INT32_ERR     _JS_TYPEINFO2(int32,             _JS_I32, --,  --, FAIL_NEG)
>+#define _JS_TYPEINFO_UINT32        _JS_TYPEINFO(uint32,             _JS_I32, --,  --)
>+#define _JS_TYPEINFO_DOUBLE        _JS_TYPEINFO(jsdouble,           _JS_F64, "",  "d")
>+#define _JS_TYPEINFO_STRING        _JS_TYPEINFO(JSString *,         _JS_PTR, "",  "s")
>+#define _JS_TYPEINFO_STRING_ERR    _JS_TYPEINFO2(JSString *,        _JS_PTR, --,  --, FAIL_NULL)
>+#define _JS_TYPEINFO_OBJECT        _JS_TYPEINFO(JSObject *,         _JS_PTR, "",  "o")
>+#define _JS_TYPEINFO_OBJECT_ERR_NULL _JS_TYPEINFO2(JSObject *,      _JS_PTR, --,  --, FAIL_NULL)
>+#define _JS_TYPEINFO_OBJECT_ERR_VOID _JS_TYPEINFO2(JSObject *,      _JS_PTR, --,  --, FAIL_VOID)
>+#define _JS_TYPEINFO_REGEXP        _JS_TYPEINFO(JSObject *,         _JS_PTR, "",  "r")
>+#define _JS_TYPEINFO_SCOPEPROP     _JS_TYPEINFO(JSScopeProperty *,  _JS_PTR, --,  --)
>+#define _JS_TYPEINFO_GUARDRECORD   _JS_TYPEINFO(nanojit::GuardRecord *,  _JS_PTR, --,  --)
>+#define _JS_TYPEINFO_INTERPSTATE   _JS_TYPEINFO(avmplus::InterpState *,  _JS_PTR, --,  --)
>+#define _JS_TYPEINFO_FRAGMENT      _JS_TYPEINFO(nanojit::Fragment *,     _JS_PTR, --,  --)

Wondering how badly overlong lines get if you align each field on the same column as all other rows' corresponding field?

> #define _JS_EXPAND(tokens)  tokens
> 
>-#define _JS_CTYPE2(ctype, size)    ctype
>+#define _JS_CTYPE2(ctype, size, pch, ach, flags)    ctype
> #define _JS_CTYPE(tyname)          _JS_EXPAND(_JS_CTYPE2 _JS_TYPEINFO_##tyname)
>-#define _JS_RETSIZE2(ctype, size)  size##_ARGSIZE
>+#define _JS_RETSIZE2(ctype, size, pch, ach, flags)  size##_ARGSIZE
> #define _JS_RETSIZE(tyname)        _JS_EXPAND(_JS_RETSIZE2 _JS_TYPEINFO_##tyname)
>-#define _JS_ARGSIZE2(ctype, size)  size##_RETSIZE
>+#define _JS_ARGSIZE2(ctype, size, pch, ach, flags)  size##_RETSIZE
> #define _JS_ARGSIZE(tyname)        _JS_EXPAND(_JS_ARGSIZE2 _JS_TYPEINFO_##tyname)
>+#define _JS_TNTYPE_PCH2(ctype, size, pch, ach, flags)  pch
>+#define _JS_TNTYPE_PCH(tyname)     _JS_EXPAND(_JS_TNTYPE_PCH2 _JS_TYPEINFO_##tyname)
>+#define _JS_TNTYPE_ACH2(ctype, size, pch, ach, flags)  ach
>+#define _JS_TNTYPE_ACH(tyname)     _JS_EXPAND(_JS_TNTYPE_ACH2 _JS_TYPEINFO_##tyname)
>+#define _JS_TNTYPE_FLAGS2(ctype, size, pch, ach, flags)  flags
>+#define _JS_TNTYPE_FLAGS(tyname)   _JS_EXPAND(_JS_TNTYPE_FLAGS2 _JS_TYPEINFO_##tyname)

Could do here too. Most SpiderMonkey macros squish out spaces in formal parameter lists to help avoid overlong lines or over-indented macro bodies, where practical.

Other than these minor nits/suggestions, looks great. If trace-tests.js is covering all these natives and it passes, get it in before someone adds another native!

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

Same tokens after preprocessing, but different names for some of the macros.  E.g. s/TYPEINFO/CTYPE/g which helps with the line lengths.

Also added this comment about errors...

/*
 * Supported types for builtin functions. 
 *
 * Types with -- for the two string fields are not permitted as argument types
 * in JS_DEFINE_TRCINFO.
 *
 * If a traceable native can fail, the values that indicate failure are part of
 * the return type:
 *     JSVAL_FAIL: JSVAL_ERROR_COOKIE
 *     BOOL_FAIL: JSVAL_TO_BOOLEAN(JSVAL_VOID)
 *     INT32_FAIL: any negative value
 *     STRING_FAIL: NULL
 *     OBJECT_FAIL_NULL: NULL
 *     OBJECT_FAIL_VOID: JSVAL_TO_OBJECT(JSVAL_VOID)
 *         (NULL means the function successfully returned JS null.)
 *
 * Special builtins known to the tracer can have their own idiosyncratic
 * error codes.
 *
 * When a traceable native returns a value indicating failure, we fall off
 * trace.  If an exception is pending, it is thrown; otherwise, we assume the
 * builtin had no side effects and retry the current bytecode in the
 * interpreter.
 * 
 * So a builtin must not return a value indicating failure after causing side
 * effects (such as reporting an error), without setting an exception pending.
 * The operation would be retried, despite the first attempt's observable
 * effects.
 */

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 343607 [details] [diff] [review]
v2

Looks good. Is it coincidence (and not required) that JSVAL_ERROR_COOKIE and the ...OBJECT_FAIL_VOID code are both 0x10?

/be

Comment 6

[Jason Orendorff [:jorendorff]]

I don't think it's required, but this patch was mostly just documenting and macroizing the status quo.  Andreas?

Comment 7

[Jason Orendorff [:jorendorff]]

Pushed.

http://hg.mozilla.org/tracemonkey/rev/dc6d3b9b9dd2