814795 - Replace macros.py with SpiderMonkey version

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Norbert Lindenberg]

The file js/src/builtin/macros.py is copied from V8 sources and contains numerous macros that only make sense in the V8 context. It should be replaced with a version that is tailored to the capabilities of the self-hosting framework in SpiderMonkey.

Comment 1

[Till Schneidereit [:till]]

I agree. We're using very little of that file and should definitely remove all those things from it that we won't or can't use.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: patch

Once bug 848634 lands, we can do use Preprocessor.py instead. This patch also turns on compression, sorry for not separating the two features.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 721986 [details] [diff] [review]
patch

Review of attachment 721986 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ so much nicer!

::: js/src/vm/SelfHosting.cpp
@@ +506,5 @@
> +            return false;
> +        }
> +#else
> +        const char *src = selfhosted::rawSources;
> +        uint32_t srcLen = selfhosted::GetRawScriptsSize();

nit: can be moved above the #ifdef and de-duplicated

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Till Schneidereit [:till] from comment #3)
> Comment on attachment 721986 [details] [diff] [review]
> patch
> 
> Review of attachment 721986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> \o/ so much nicer!
> 
> ::: js/src/vm/SelfHosting.cpp
> @@ +506,5 @@
> > +            return false;
> > +        }
> > +#else
> > +        const char *src = selfhosted::rawSources;
> > +        uint32_t srcLen = selfhosted::GetRawScriptsSize();
> 
> nit: can be moved above the #ifdef and de-duplicated

srcLen can be, but src can't due to its scope freeing.

Comment 5

[Till Schneidereit [:till]]

> srcLen can be, but src can't due to its scope freeing.

Sure, that's what I meant. Micro-nit, I know.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: patch

Uses actual CPP over Preprocessor changes.

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 725580 [details] [diff] [review]
patch

Review of attachment 725580 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. r=me and rejoice=me with nits addressed.

::: js/src/builtin/embedjs.py
@@ +52,5 @@
> +def ToCArray(lines):
> +  result = []
> +  for chr in lines:
> +    result.append(str(ord(chr)))
> +  return ", ".join(result)

I'm not sure we even need to include the license for these two short functions. Doesn't do any harm, though, so I guess we don't need to bother Gerv with that question.

@@ +154,5 @@
> +               help='C array header file')
> +  p.add_option('-s', type='string', metavar='jsfilename', default='selfhosted.js',
> +               help='Combined postprocessed JS file')
> +  (options, sources) = p.parse_args()
> +  if not (options.o and options.s and options.m and options.p and sources):

No need to check for anything but options.p and sources: the other options have defaults.

@@ +157,5 @@
> +  (options, sources) = p.parse_args()
> +  if not (options.o and options.s and options.m and options.p and sources):
> +    p.print_help()
> +    exit(1)
> +  cpp = options.p.split(' ')

This won't work if the path to cpp contains spaces. Not very likely, but still better to use shlex.split.

::: js/src/vm/SelfHosting.cpp
@@ +498,4 @@
>          uint32_t srcLen = selfhosted::GetRawScriptsSize();
> +
> +#ifdef USE_ZLIB
> +        const unsigned char *compressed = selfhosted::compressedSources;

Might as well add `using namespace selfhosted` to the file and get rid of all these.

Comment 8

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/475c1655ce61

Comment 10

[Shu-yu Guo [:shu]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=0219f1b3181f

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/475c1655ce61

Comment 12

[Norbert Lindenberg]

Attachment: Replace remaining use of V8 macro

The main patch left one use of a V8 macro behind, thus breaking the internationalization API. Here's the fix.

Comment 13

[Till Schneidereit [:till]]

Comment on attachment 727832 [details] [diff] [review]
Replace remaining use of V8 macro

Review of attachment 727832 [details] [diff] [review]:
-----------------------------------------------------------------

stealing

Comment 14

[Till Schneidereit [:till]]

Comment on attachment 727832 [details] [diff] [review]
Replace remaining use of V8 macro

and checkin: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb5a80119674

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Norbert Lindenberg from comment #12)
> Created attachment 727832 [details] [diff] [review]
> Replace remaining use of V8 macro
> 
> The main patch left one use of a V8 macro behind, thus breaking the
> internationalization API. Here's the fix.

Ah, I didn't catch that one, and no tests failed.

Comment 16

[Norbert Lindenberg]

That's because my tests haven't landed yet. On my side, it was a near-perfect hit: 130 of the 142 conformance tests failing, and jstests not even starting because they failed during the evaluation of the reftest manifest :-)

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eb5a80119674