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

VERIFIED FIXED in mozilla1.0

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

Trunk
mozilla1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

/be
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: mozilla1.0
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 1

16 years ago
Created attachment 73473 [details] [diff] [review]
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 on attachment 73473 [details] [diff] [review]
proposed fix/cleanup patch (diff -wu format)

Ah, spring cleaning.  r=shaver.
Attachment #73473 - Flags: review+

Comment 3

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

sr=jband
Attachment #73473 - Flags: superreview+
(Assignee)

Updated

16 years ago
Priority: -- → P1

Updated

16 years ago
Attachment #73473 - Flags: approval+

Comment 4

16 years ago
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
(Assignee)

Comment 5

16 years ago
Fixed.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 6

16 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.