Closed Bug 325951 Opened 16 years ago Closed 15 years ago

Simplify jsconfig.h

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: brendan, Assigned: brendan)

References

()

Details

Attachments

(7 files, 1 obsolete file)

This file goes back more than 8 years, and we don't need the complexity it brings. Proposals:

* Get rid of all the JS_BUG_* ancient history macros.
* Drop support for JS1.4 and earlier.
* Eliminate JS_HAS_CALL_OBJECT and other must-haves.

Comments?

/be
Flags: testcase-
I'll want to fix this before landing iterators and generators, to avoid impossible configurability for things such as JS_HAS_EXCEPTIONS (which will be required by the for-in loop code, period full stop).

/be
Blocks: geniter
Status: NEW → ASSIGNED
Priority: -- → P1
Attached patch proposed fixSplinter Review
Lots of systematic changing, by hand alas (Dave Yost's unifdef can't handle #if, and my old SGI unifdef which could is apparently lost behind SGI's firewall). Get out your rubber stamp! ;-)

/be
Attachment #219820 - Flags: review?(mrbkap)
This is easier to read in places.

/be
Attached patch spidermonkey-n.tests.patch (obsolete) — Splinter Review
obsolete the js1_1,...js1_4 tests.
Attachment #219847 - Flags: review?(brendan)
Comment on attachment 219820 [details] [diff] [review]
proposed fix

In all likely hood, I'm missing something, but this looks fine.
Attachment #219820 - Flags: review?(mrbkap) → review+
Bob, do we have split tests for the ECMA-262 Edition 3 behavior elsewhere, and so on?  Also, the *proto* tests under js1_3 -- are they __proto__ and therefore something we want to test still?  In general I'm concerned that tests were put under a js1_* directory that, in light of the final standard, should be under an ecma3 directory.

/be
I turned off -pu8 for my diff options in .cvsrc, generated the flat diff, deleted all lines matching /^< #if JS_/ and /^< #endif/, went through the residuum by hand and found it all good.

/be
Attachment #219920 - Flags: review+
(In reply to comment #6)
> Bob, do we have split tests for the ECMA-262 Edition 3 behavior elsewhere, and
> so on?  

"split tests" ? as in String.prototype.split? Most of those are in ecma/String and ecma_2/String.

> Also, the *proto* tests under js1_3 -- are they __proto__ and therefore
> something we want to test still? 

Yes, several _proto_ related tests are in js1_3/inherit/

>  In general I'm concerned that tests were put
> under a js1_* directory that, in light of the final standard, should be under
> an ecma3 directory.

There are 121 test files in js1_*. 85 of those are in js1_2 which I doubt we care about many of them. 12 of the 24 tests in js1_3 relate to inheritance with the others scattered around.

I could run the tests without setting the version and only remove those that fail when not run with the appropriate version (e.g javascript1.2 for js1_2). That would eliminate the version dependent results while retaining the tests for daily testing. Ok?
(In reply to comment #8)
> I could run the tests without setting the version and only remove those that
> fail when not run with the appropriate version (e.g javascript1.2 for js1_2).
> That would eliminate the version dependent results while retaining the tests
> for daily testing. Ok?

That sounds like a great idea.

/be
(In reply to comment #8)
> There are 121 test files in js1_*.
That should have read 121 tests files in js1_1 - js1_4.

(In reply to comment #8)
Will do.
Fix checked in.

/be
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
#if math is hard, let's go shopping.

Should be all better in a tinder-cycle.

/be
Attachment #219847 - Attachment is obsolete: true
Attachment #219847 - Flags: review?(brendan)
Comment on attachment 219939 [details] [diff] [review]
missing jskeyword.tbl patch + jsparse.c fix

>--- jskeyword.tbl	26 Jan 2006 08:47:50 -0000	3.3
>-#if JS_HAS_INSTANCEOF
> JS_KEYWORD(instanceof,  TOK_INSTANCEOF, JSOP_INSTANCEOF,JSVERSION_1_4)
>-#else
>-JS_KEYWORD(instanceof,  TOK_RESERVED,   JSOP_NOP,       JSVERSION_1_4)
>-#endif
> 

Should JSVERSION_1_4 be changed to JSVERSION_DEFAULT since JSVERSION_1_4 is no longer supported and similarly for export, enum and debugger? And since AFAIK the keyword set is not going to be extended with JS2.0, perhaps it makes sence to remove the version argument and the checks in the scanner alltogether?
We're currently thinking about "unreserving" keywords in property identifier contexts (in object initialisers, and after the . and .. operators).  But we may need to reserve some operators for Edition 4, so I think we'll want the keyword version facility.

Igor, could you patch away the low-numbered versions if you are so inclined, or at least file a bug for later?  I've got a full plate and am trying to move on (after I torched the tree twice today -- what a vandal!).  Thanks for any help.

/be
Checking in spidermonkey-n.tests;
/cvsroot/mozilla/js/tests/spidermonkey-n.tests,v  <--  spidermonkey-n.tests
new revision: 1.9; previous revision: 1.8
done
Attachment #219988 - Flags: review?(mrbkap)
Attachment #219988 - Flags: review?(mrbkap) → review+
Even before config changes JS_HAS_SPARSE_ARRAYS was always defined to 0 and was not used anywhere outside jsconfig.c. This patch removes this. 

There is another define, JS_HAS_FUN_EXPR_STMT, that is not used outside jsconfig.h. But at least it is defined to be different depending on the version even after the config changes. Is it a bug that jsparse.c does not check for it?



The config cleanups made JS_HAS_SPARSE_ARRAYS to be defined always as 0.  Since it is no longer used,
The patch to jsarray.c regressed splice, see bug 326466 comment 35.

I promise not to inflict runtime versionitis on the engine, ever again.

/be
Blocks: 335964
No longer blocks: 335964
FYI. This appears to have broken IE View.  ANnattempt to launch IE via IE View now results in a pop-up saying "TypeError: arguments.push is not a function".

I filed IE View bug number 14065 on this issue.
That sounds like bug 336100.
No longer blocks: 336100
Depends on: 336100
(In reply to comment #20)
> That sounds like bug 336100.

Which was not due to this bug, but to bug 304376, right?

/be
(In reply to comment #21)
> Which was not due to this bug, but to bug 304376, right?

A combination: this patch made js_CallClass use JSCLASS_IS_ANONYMOUS (introduced in that bug).
(In reply to comment #22)
> (In reply to comment #21)
> > Which was not due to this bug, but to bug 304376, right?
> 
> A combination: this patch made js_CallClass use JSCLASS_IS_ANONYMOUS
> (introduced in that bug).

Why don't we back that one line change out, for now?  r=me, let's help unregress for the moment.  We should track this flappage with a separate bug.

/be
(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Which was not due to this bug, but to bug 304376, right?
> > 
> > A combination: this patch made js_CallClass use JSCLASS_IS_ANONYMOUS
> > (introduced in that bug).
> 
> Why don't we back that one line change out, for now?  r=me, let's help
> unregress for the moment.  We should track this flappage with a separate bug.
> 
I verified with my own build that backing out that one line does fix the IE View issue.
> /be
> 

You need to log in before you can comment on or make changes to this bug.