Closed Bug 808286 Opened 7 years ago Closed 7 years ago

Use an enum instead of JSBool for charArgs parameter of js_ReportErrorNumberVA

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: emk, Assigned: edransch)

Details

(Whiteboard: [js:t][good first bug][mentor=Waldo][lang=c++])

Attachments

(1 file, 3 obsolete files)

Quoting from bug 801487:
> ::: js/src/jsapi.cpp
> @@ +6732,5 @@
> > +                         va_list ap)
> > +{
> > +    AssertHeapIsIdle(cx);
> > +    js_ReportErrorNumberVA(cx, JSREPORT_ERROR, errorCallback, userRef,
> > +                           errorNumber, JS_FALSE, ap);
> 
> The JS_FALSE here (which I would ordinarily say should be false, just so
> you're aware for other patches) is totally unintelligible when read here. 
> :-)
> 
> These days we would make this an enum ArgumentType { AsciiString,
> UnicodeString } so that places like this could pass something readable, and
> more obviously correct or not-correct.  Could you file a followup bug to
> make this change, please?  Could be good first bug material, if I don't get
> irritated enough by this to just go do it myself.  :-)
Whiteboard: [good first bug][mentor=Waldo][lang=c++]
Whiteboard: [good first bug][mentor=Waldo][lang=c++] → [js:t][good first bug][mentor=Waldo][lang=c++]
Attached patch Patch for feedback (obsolete) — Splinter Review
I'm uploading this patch to check if I'm on the right track for this, or if you've got something else in mind.

In the enum: 
ECA_ASCII_ARGS corresponds to the previous TRUE value, meaning that js_ExpandErrorArguments ( http://mxr.mozilla.org/mozilla-central/source/js/src/jscntxt.cpp#956 ) needs to inflate the string.

ECA_UNICODE_ARGS corresponds to FALSE, meaning no expansion is necessary.
Assignee: general → edransch.contact
Attachment #684545 - Flags: feedback?(jwalden+bmo)
Comment on attachment 684545 [details] [diff] [review]
Patch for feedback

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

This looks broadly on the right track.

Given we're changing a bool to an enum, we want to catch all places where we might have been testing these values bool-wise and change them to enum-wise equality testing.  The easiest, and safest, way to do that is to change the names of all the variables/fields of that type, then fix up each use correctly, knowing the compiler will catch every such site.  Conveniently, I think we have good reason even beyond this to rename all these, so eliminating test-as-bool should just work when you rename the variables/fields.

::: js/src/frontend/TokenStream.cpp
@@ -498,5 @@
>      err->report.filename = filename;
>      err->report.originPrincipals = originPrincipals;
>      err->report.lineno = tp->begin.lineno;
>  
> -    err->hasCharArgs = !(flags & JSREPORT_UC);

I think you're missing a hasCharArgs around line 446 in this file that needs changing (as should be pointed out when you rename this field).

::: js/src/frontend/TokenStream.h
@@ +454,5 @@
>  struct CompileError {
>      JSContext *cx;
>      JSErrorReport report;
>      char *message;
> +    JSErrorCharArgs hasCharArgs;

This field name is now not quite right, basically for the reasons the argument names later are not quite right.  Let's rename this to argumentsType as well.

@@ +459,2 @@
>      CompileError(JSContext *cx)
> +     : cx(cx), message(NULL), hasCharArgs(ECA_UNICODE_ARGS)

This was pre-existing, but if you're here, we might as well fix it -- plus it gets you more up to speed on SpiderMonkey formatting conventions.  The : here should be indented two spaces past the constructor name, so add one more space here.

::: js/src/jscntxt.h
@@ +1936,5 @@
>  } /* namespace js */
>  
> +enum JSErrorCharArgs {
> +    ECA_UNICODE_ARGS,
> +    ECA_ASCII_ARGS

I would name these ArgumentsAreUnicode and ArgumentsAreASCII -- these read much better.  Also we're kind of moving to a more modern InterCaps enum-naming style, which these names would fit better.

This shouldn't use the JS prefix -- that's mostly for things that are exposed in the JSAPI.  In contrast this is mentioned in a purely internal header.  Let's name it ErrorArgumentsType instead.

@@ +1946,5 @@
>  
>  extern JSBool
>  js_ReportErrorNumberVA(JSContext *cx, unsigned flags, JSErrorCallback callback,
>                         void *userRef, const unsigned errorNumber,
> +                       JSErrorCharArgs charArgs, va_list ap);

I would name this argument (and the other -- and make sure to change the function definition sites, not just these declarations) argumentsType.  "charArgs" named a characteristic that was either true or false, when this was a boolean.  It doesn't quite fit so well now, in my opinion.
Attachment #684545 - Flags: feedback?(jwalden+bmo)
Attached patch Patch for review (obsolete) — Splinter Review
Thanks for the feedback!

I agree that this naming convention is more intuitive.
 
The try run for this patch is available at https://tbpl.mozilla.org/?tree=Try&rev=c32baadbec51

I'm not sure why the b2g otoro build is red, it doesn't look related to this patch, but I'll continue to investigate.
Attachment #684545 - Attachment is obsolete: true
Attachment #686933 - Flags: review?(jwalden+bmo)
Comment on attachment 686933 [details] [diff] [review]
Patch for review

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

Just one little style thing I should have pointed out earlier, and one substantive thing -- but almost there!

::: js/src/jscntxt.cpp
@@ +1130,5 @@
>  
>      char *message;
>      va_list dummy;
>      if (!js_ExpandErrorArguments(cx, callback, userRef, errorNumber,
> +                                 &message, &report, ArgumentsAreASCII, dummy)) {

Um, shouldn't this be ArgumentsAreUnicode?

::: js/src/jscntxt.h
@@ +1959,5 @@
>  
> +enum ErrorArgumentsType {
> +    ArgumentsAreUnicode,
> +    ArgumentsAreASCII
> +};

Hmm, actually, I should have been more precise.  We use the JS prefix for things that are part of the JSAPI; we used to use js_ as the prefix for things not part of it but shared, but now we use no prefix but put them in the js namespace.  So could you move this up a few lines and add js:: to the relevant places, please?
Attachment #686933 - Flags: review?(jwalden+bmo) → review-
Attached patch Patch for review (obsolete) — Splinter Review
Thanks for catching wrong parameter!

Moved the enum into the js namespace.

Try builds are available at https://tbpl.mozilla.org/?tree=Try&rev=4cddbf0291d7

Is there a set of tests I can run on try to ensure that behaviour is unchanged? (I'd assume that jsreftests would be a good idea, are there other relevant tests as well?)
Attachment #686933 - Attachment is obsolete: true
Attachment #687429 - Flags: review?(jwalden+bmo)
Comment on attachment 687429 [details] [diff] [review]
Patch for review

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

Looks good!  Do you want to push this, with the one change, or should I?

The set of tests we have to run is pretty much jstests, jit-tests, and jsapi-tests.  (jsreftests are identical to jstests, except they run in the browser.  Oh, and some tests in each check for things like running in the browser versus not, so technically for full coverage you have to run jstests with the shell and jsreftests in the browser both.  But for most patches it's not truly necessary.  And anything you'd write a browser-specific test for, really should be written so that it can run in the shell too, perhaps by adding helper methods or whatever to the shell.)  They're run these ways:

  # in js/src
  python tests/jstests.py path/to/shell
  python jit-test/jit_test.py path/to/shell
  # from wherever
  objdir/jsapi-tests/jsapi-tests

jsapi-tests are always going to be special like this, because they're compiled-code tests mostly specifically designed to exercise JSAPI methods and features.  The other two are separate for somewhat historical reasons; we might unify them at some point, but who knows.  For random JS engine changes like here, that don't touch the JITs, jstests are more likely to point out issues.  For changes that require JIT changes, jit-tests are possibly more likely to point out issues.  (And for changes where you need to make JIT changes, but haven't, only jit-tests are likely to point them out.)

Your try run hit just B, which corresponds to running jit-tests and jstests and jsapi-tests -- but not in the browser.  This change is simple enough that I don't think it needs much browser testing, so I think it's good to go.  Deeper changes I'd want more, but for this, this is good.

::: js/src/frontend/TokenStream.h
@@ +454,5 @@
>  struct CompileError {
>      JSContext *cx;
>      JSErrorReport report;
>      char *message;
> +    js::ErrorArgumentsType argumentsType;

If this is inside the js namespace or in a namespace nested inside it (which it is, it's in js::frontend), it's not necessary to have js:: here, and we usually wouldn't.  The only exception would be if the type existed in both js and js::frontend as different types, and you needed the js:: type.  (Although, we'd probably try to figure out something other than having the same name in both namespaces, rather than have that happen.)
Attachment #687429 - Flags: review?(jwalden+bmo) → review+
(In reply to Erick Dransch [:edransch] from comment #5)
> Is there a set of tests I can run on try to ensure that behaviour is
> unchanged? (I'd assume that jsreftests would be a good idea, are there other
> relevant tests as well?)

Hmm, I wasn't quite responsive to this.  B gets you the various test suites, jsreftest gets you many of those in browsers.  Past that you only get the incidental testing from all the other tests we have, in terms of how they use JavaScript.  Which can be valuable -- and indeed, for scary changes these often point out stuff -- but if we have a bug that only manifests itself in a script in some other test suite, we should write a corresponding test that works with the JS shell.  For simple patches, B+jsreftest is good enough.
Thank you very much for the overview! Very informative; I appreciate it.

The only difference between this patch and the previous one is that I've removed the |js::| from references within the |js| namespace.

The try run here includes jsreftests:
https://tbpl.mozilla.org/?tree=Try&rev=f196d85bf026

I'd appreciate it if you could check this in if you think it's ready (I have L1 access). Thanks :)
Attachment #687429 - Attachment is obsolete: true
Attachment #690684 - Flags: checkin?(jwalden+bmo)
FYI, checkin-needed is what you really want. Thanks for the patch!
Keywords: checkin-needed
This is queued up in a tree with another patch I have to push, will do so once I've made the tweaks I need to to that other patch.  Just in case you were wondering.  :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e283fe34dcb1
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla20
Comment on attachment 690684 [details] [diff] [review]
Patch for checkin

Landed, and thanks again for the patch!
Attachment #690684 - Flags: checkin?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/e283fe34dcb1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.