129972 - strict warnings without the strict option, and other 1.0 JS engine tidying

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Patch in a minute, fixing the spurious strict warning problem, and making the
other cleanups, fall into the "wanted for 1.0" category.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix/cleanup patch (diff -wu format)

- Consolidate JS_HAS_STRICT_OPTION testing inside the three error reporters,
  js_ReportErrorVA, js_ReportErrorNumberVA, and js_ReportCompileErrorNumber,
  instead of testing in callers.  A few callers failed to test, leading to
  strict warnings even when the strict option had not been selected, e.g.,
  the "useless expression" strict warning in jsemit.c.

  Some callers still check JS_HAS_STRICT_OPTION to avoid some extra overhead
  setting up a call to an error reporter that will suppress the strict report
  anyway, in the new scheme.

- Remove spurious jshash.h include from jslock.h.

- Use js_ReportCompileErrorNumber in jsscan.c when giving the strict warning
  about deprecated sharp variable usage.

- The big patch for bug 62164 contained this gaffe: scope->hashShift is set
  but never used!  A left-over from an earlier stage of the patch for that
  bug, when the code stored only sizeLog2 and derived hashShift.  This change
  affected only js_SearchScope, and I took the opportunity to registerize
  sizeLog2 as well.  After mozilla1.0 it's likely that sizeLog2 will go away
  to make room for a uint16 flags member, and sizeLog2 users will derive it
  (via a SCOPE_CAPACITY macro) from hashShift.

- Nit: use continue rather than empty statement for empty loop body.
- Nit: remove extern qualifier from function definition, a few crept over the
  years and recently due to copy/paste from header file prototypes.

- Nit: fix up the big span-dependent instruction comment in jsemit.c to tell
  the bugzilla URL that tracked the spandep work, instead of talking about
  "this patch" (wording that came from the checkin message).
- Nit: factor common predicate out of two successive if conditions in
  js_SetJumpOffset.

- Nit: tab elimination in jsexn.c, jsgc.c, jsscan.c, and jsscan.h.
- Nit: use intN, not NSPR2-ish intn (from PRUintn), in one place in jsfun.c.
- Nit: comment English usage fix in jsgc.c.
- Nit: no gratuitous parens around hex constants in jsscan.h; added a comment
  there for js_ReportCompileErrorNumber, too.

/be

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 73473 [details] [diff] [review]
proposed fix/cleanup patch (diff -wu format)

Ah, spring cleaning.  r=shaver.

Comment 3

[John Bandhauer]

Comment on attachment 73473 [details] [diff] [review]
proposed fix/cleanup patch (diff -wu format)

sr=jband

Comment 4

[Asa Dotzler [:asa]]

Comment on attachment 73473 [details] [diff] [review]
proposed fix/cleanup patch (diff -wu format)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 5

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 6

[Phil Schwartau]

Marking Verified FIXED.

Before this fix, I could obtain a strict warning as follows,
without invoking the -s strict option in the JS shell: 

[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js
js> function f(){1,2}
1: strict warning: useless expression


But now, I must invoke the -s option in order to get the warning:

[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js
js> function f() {1,2}
js>

[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js -s
js> function f() {1,2}
1: strict warning: useless expression


Brendan, if there are any other things like this that I should check,
please let me know - thanks.