Closed Bug 823978 Opened 7 years ago Closed 6 years ago

rename JSOPTION_STRICT to JSOPTION_EXTRA_WARNINGS

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Benjamin, Assigned: Benjamin)

Details

Attachments

(1 file)

JSOPTION_STRICT enables warnings extra warnings during compilation. It is not related to ES5 strict mode except that it triggers warnings about thing that are errors in ES5 strict mode. The avoid confusion of the two, JSOPTION_STRICT should be renamed to JSOPTION_LINT.
Umm, "lint" also has a specific meaning that doesn't have much to do with JSOPTION_STRICT, and there is already a JSLint that has nothing to do with JSOPTION_STRICT.  How about JSOPTION_EXTRA_WARNINGS?
As jorendorff said in bug 823310 c11
("JSOPTION_LINT" sounds delightful. Unfortunately there is also a thing called jslint, but at least that is outside our codebase! (also, it should be dying off, since the jshint fork.))
There's a javascript.options.strict option in about:config that enables strict warnings.  Any plans to change that as well?  I don't know how difficult it is to rename about:config options...
(And I suspect re-using "lint" will cause confusion, as bhackett says.  Even if jslint is dying, many people still know about it.)
Considering nobody wants to keep it around, I propose we just nix it.
Nobody wants to keep "javascript.options.strict" around?  Definitely people do.  There are addon developer docs up the wazoo that tell developers about it, as a suggestion for how to diagnose issues.  I don't think I'd change that name, myself.  The JSOPTION_*/preference mismatch is perhaps unfortunate, but eh.  Browser users have the preference API.  What underlies it can be a different name without them caring.
(In reply to :Benjamin Peterson from comment #5)
> Considering nobody wants to keep it around, I propose we just nix it.

I find the JSOPTION_STRICT option extremely useful.
Okay, we'll have to keep it. However, it can still renamed. JSOPTION_LINT is apparently out so maybe JSOPTION_SYNTAX_WARNINGS?
JSOPTION_STRICT controls more than just syntax warnings; there are some runtime warnings underneath it as well.
JSOPTION_EXTRA_WARNINGS, JSOPTION_MORE_WARNINGS?
The court of IRC now likes JSOPTION_EXTRA_WARNINGS.
Summary: rename JSOPTION_STRICT to JSOPTION_LINT → rename JSOPTION_STRICT to JSOPTION_EXTRA_WARNINGS
Attached patch renameSplinter Review
This renames the option internally. I didn't attempt to deal with the places where its exposed as strict like in prefs.
Attachment #760760 - Flags: review?(jwalden+bmo)
Comment on attachment 760760 [details] [diff] [review]
rename

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

I assume we'll be cleaning up "strict" mentions that refer to this for years.  Might as well get the other cases I see in context here, tho, if you're changing this stuff.

::: js/src/frontend/Parser.cpp
@@ +425,5 @@
>      cx->runtime()->activeCompilations++;
>  
>      // The Mozilla specific 'strict' option adds extra warnings which are not
>      // generated if functions are parsed lazily. Note that the standard
>      // "use strict" does not inhibit lazy parsing.

This should mention JSOPTION_EXTRA_WARNINGS, not the "strict" option.

@@ +2887,1 @@
>          if (!parser->report(ParseStrictWarning, false, pn, JSMSG_VAR_HIDES_ARG, bytes.ptr()))

Looks like ParseStrictWarning might want renaming, using "strict" nomenclature as it does.

::: js/src/jscntxt.cpp
@@ +542,2 @@
>          /*
>           * Error in strict code; warning with strict option; okay otherwise.

Comment fix!

@@ +552,5 @@
>          else
>              return true;
>      } else if (JSREPORT_IS_STRICT(*flags)) {
>          /* Warning/error only when JSOPTION_STRICT is set. */
> +        if (!cx->hasExtraWarningsOption())

Comment needs fixing!

::: js/src/jsobj.cpp
@@ +3934,5 @@
>                  return false;
>              }
>  
>              /* Don't warn if not strict or for random getprop operations. */
> +            if (!cx->hasExtraWarningsOption() || (op != JSOP_GETPROP && op != JSOP_GETELEM))

Don't warn if we're not generating extra warnings, or for random getprop operations.

@@ +4155,5 @@
>          if (!script)
>              return true;
>  
>          /* If neither cx nor the code is strict, then no check is needed. */
> +        if (!script->strict && !cx->hasExtraWarningsOption())

Comment here could use tweaking a little.  This, or something:

If the script's not strict mode and we're not generating extra warnings, no check is needed.

@@ +4178,5 @@
>          if (!script)
>              return true;
>  
>          /* If neither cx nor the code is strict, then no check is needed. */
> +        if (!script->strict && !cx->hasExtraWarningsOption())

Same comment as before.

@@ +4324,5 @@
>          } else {
>              JS_ASSERT(shape->isDataDescriptor());
>  
>              if (!shape->writable()) {
>                  /* Error in strict mode code, warn with strict option, otherwise do nothing. */

warn when extra warnings are enabled

@@ +4401,5 @@
>          return ArraySetLength(cx, obj, id, attrs, vp, strict);
>  
>      if (!shape) {
>          if (!obj->isExtensible()) {
>              /* Error in strict mode code, warn with strict option, otherwise do nothing. */

warn when extra warnings are enabled
Attachment #760760 - Flags: review?(jwalden+bmo) → review+
Thanks for carefully point out what I missed there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e69e102a432
Assignee: general → benjamin
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/cf1461d6234c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Benjamin, could you update https://developer.mozilla.org/en-US/docs/SpiderMonkey/24 for this, please?  Also the wiki pages for JSOPTION_STRICT and whatever else.
Pages fixed (except for out-of-date looking intl ones.)
You need to log in before you can comment on or make changes to this bug.