Closed Bug 829942 Opened 7 years ago Closed 6 years ago

Make jit_test.py Py3k-readier

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Details

Attachments

(1 file)

Attached patch Patch v1Splinter Review
No description provided.
Attachment #701460 - Flags: review?(jhammel)
Attachment #701460 - Flags: review?(choller)
Comment on attachment 701460 [details] [diff] [review]
Patch v1

Looks good to me, but deferring review to terrence because I'm not a JS reviewer :)
Attachment #701460 - Flags: review?(terrence)
Attachment #701460 - Flags: review?(choller)
Attachment #701460 - Flags: feedback+
Comment on attachment 701460 [details] [diff] [review]
Patch v1

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

::: js/src/jit-test/jit_test.py
@@ +161,5 @@
>      scriptdir_var = os.path.dirname(path);
>      if not scriptdir_var.endswith('/'):
>          scriptdir_var += '/'
> +    consts = (('platform', sys.platform), ('libdir', libdir_var), ('scriptdir', scriptdir_var))
> +    expr = '; '.join(define_const(k,v) for (k,v) in consts)

This is super-gross and still not adequate for the general case: e.g. what if the scriptdir contains quotes? Just encode the string quotes directly in the format string and use |str.format()| for python3 compatibility.

@@ -575,5 @@
>      max_jobs_default = 1
>      if HAVE_MULTIPROCESSING:
>          try:
>              max_jobs_default = cpu_count()
> -            print "Defaulting to %d jobs in parallel" % max_jobs_default

Thank you!

@@ +760,5 @@
>          if not ok:
>              sys.exit(2)
>      except OSError:
>          if not os.path.exists(JS):
> +            print("JS shell argument: file does not exist: '%s'"%JS, file=sys.stderr)

Use str.format() here.
Attachment #701460 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #2)
> Comment on attachment 701460 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 701460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/jit_test.py
> @@ +161,5 @@
> >      scriptdir_var = os.path.dirname(path);
> >      if not scriptdir_var.endswith('/'):
> >          scriptdir_var += '/'
> > +    consts = (('platform', sys.platform), ('libdir', libdir_var), ('scriptdir', scriptdir_var))
> > +    expr = '; '.join(define_const(k,v) for (k,v) in consts)
> 
> This is super-gross and still not adequate for the general case: e.g. what
> if the scriptdir contains quotes? Just encode the string quotes directly in
> the format string and use |str.format()| for python3 compatibility.

Will do.

> @@ -575,5 @@
> >      max_jobs_default = 1
> >      if HAVE_MULTIPROCESSING:
> >          try:
> >              max_jobs_default = cpu_count()
> > -            print "Defaulting to %d jobs in parallel" % max_jobs_default
> 
> Thank you!

Decoder secretly already landed that :)
Comment on attachment 701460 [details] [diff] [review]
Patch v1

what terrence said. I would have tended to have define_const => define_consts and take all the arguments, returning `expr = '; '.join(define_const(k,v) for (k,v) in consts)` from that function, but no big deal
Attachment #701460 - Flags: review?(jhammel) → review+
Ms2ger: did you forget to land this r+'d patch from January 2013? :)
Flags: needinfo?(Ms2ger)
I think I hit some issue on try at the time... That seems to have been magically fixed, though, so I'll land it.
Flags: needinfo?(Ms2ger)
https://hg.mozilla.org/mozilla-central/rev/1971bf0f83eb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.