Closed Bug 814795 Opened 7 years ago Closed 7 years ago

Replace macros.py with SpiderMonkey version

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: mozillabugs, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
Depends on: 848634
Attached patch patch (obsolete) — Splinter Review
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 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
(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.
> srcLen can be, but src can't due to its scope freeing.

Sure, that's what I meant. Micro-nit, I know.
Attached patch patchSplinter Review
Uses actual CPP over Preprocessor changes.
Attachment #721986 - Attachment is obsolete: true
Attachment #725580 - Flags: review?(tschneidereit)
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.
Attachment #725580 - Flags: review?(tschneidereit) → review+
Duplicate of this bug: 824112
https://hg.mozilla.org/mozilla-central/rev/475c1655ce61
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
The main patch left one use of a V8 macro behind, thus breaking the internationalization API. Here's the fix.
Attachment #727832 - Flags: review?(shu)
Attachment #727832 - Flags: checkin?(shu)
Comment on attachment 727832 [details] [diff] [review]
Replace remaining use of V8 macro

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

stealing
Attachment #727832 - Flags: review?(shu) → review+
Comment on attachment 727832 [details] [diff] [review]
Replace remaining use of V8 macro

and checkin: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb5a80119674
Attachment #727832 - Flags: checkin?(shu) → checkin+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
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 :-)
https://hg.mozilla.org/mozilla-central/rev/eb5a80119674
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 914511
You need to log in before you can comment on or make changes to this bug.