391488 - Remove all WIN16 code under js directory

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jonathan Steele]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080815 Minefield/3.0a8pre
Build Identifier: 

Eliminate all WIN16 code in jsstddef.h.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Serge Gautherie (:sgautherie)]

What about </js2/src/*>, </js/jsd/*>, </js/src/*> ?

Comment 2

[Serge Gautherie (:sgautherie)]

All <js*>:
http://mxr.mozilla.org/mozilla-central/search?string=win16&find=/js

<js2> exists in cvs but not in Hg.

Comment 3

[Jonathan Steele]

Attachment: Tell it so

Untestes

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 353942 [details] [diff] [review]
Tell it so

I'm more than ok with this, just would appreciate Jim's blessing (or Benjamin's). Also, we should have a bug to get rid of jsstddef.h and PTRDIFF usage -- unless there is any good reason to have our own always-included-first-in-.cpp-files header.

/be

Comment 5

[Brendan Eich [:brendan]]

BTW, no super-review for SpiderMonkey.

/be

Comment 6

[Jim Blandy :jimb]

Comment on attachment 353942 [details] [diff] [review]
Tell it so

This is great --- but now a lot of it has been addressed in other ways: the patch landed for bug 465640 makes some of this patch stale; I exect bug 260538 to be re-landed, which will do the same for other parts.

The only piece of the patch which I think will still be relevant is the change to jsstddef.h.  There, I have to say I'd rather see the PTRDIFF macro removed altogether and its uses simplified than have it remain, with a single trivial definition.  Not thrilling work, but that's where the readability win would be.

Comment 7

[Jim Blandy :jimb]

Typo: that second bug is bug 269538.

Comment 8

[Jonathan Steele]

Attachment: Remove PTRDIFF

I did remove all prtdiff defined with jsstddef.h too. I didn't remove other js files that just have jsstddef.h.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 359113 [details] [diff] [review]
Remove PTRDIFF

The change to line 920 of jsscan.cpp has a typo: it should use the minus operator, not the comma operator.

On line 1512 of js.cpp, the subtraction needs parentheses around it, so that the cast to uintN will apply to the result of the subtraction, not just its left operand.

Almost all the files that used to #include <stddef.h> indirectly via "jsstddef.h" will still get it via "jstypes.h".  The exception is jskwgen.cpp; would it be better to replace the #inclusion of jsstddef.h in jskwgen.cpp with an #inclusion of <stddef.h> directly, rather than just deleting the #inclusion?

In the definition of OPDEF in jstracer.cpp, it would be nicer to simply pull the two former operands of PTRDIFF up onto their own line.

Comment 10

[Jonathan Steele]

Attachment: part 2

#1,2 and 3 are done. I look at #4 later.

Comment 11

[Jim Blandy :jimb]

(In reply to comment #10)
> #1,2 and 3 are done. I look at #4 later.

#1 and #2 look good --- thanks.

Is #3 done in this patch?  It seems that jskwgen.cpp still just has its #inclusion of jsstddef.h deleted.  It should #include <stddef.h> instead.

Comment 12

[Jonathan Steele]

Attachment: part 3

I added part 2 with #include <stddef.h> in jskwgen.cpp.

Comment 13

[Jim Blandy :jimb]

Part 3 looks good, just need that point #4 taken care of.

Comment 14

[Jonathan Steele]

Attachment: patch 4

I hope this patch is exactly you wants. I did move it to own line.

Comment 15

[Jim Blandy :jimb]

With this patch, jstrace.cpp no longer compiles:

../jsopcode.tbl: In static member function ‘static JSMonitorRecordingStatus TraceRecorder::monitorRecording(JSContext*, TraceRecorder*, JSOp)’:
../jsopcode.tbl:111: error: ‘target’ was not declared in this scope

I think what's needed here is something like this:

diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
--- a/js/src/jstracer.cpp
+++ b/js/src/jstracer.cpp
@@ -4210,8 +4210,7 @@ TraceRecorder::monitorRecording(JSContex
             js_Disassemble1(cx, cx->fp->script, cx->fp->regs->pc,             \
                             (cx->fp->imacpc)                                  \
                             ? 0                                               \
-                            : cx->fp->regs->pc -                              \
-                                      cx->fp->script->code,                   \
+                            : cx->fp->regs->pc - cx->fp->script->code,        \
                             !cx->fp->imacpc, stdout);)                        \
         flag = tr->record_##x();                                              \
         if (x == JSOP_ITER || x == JSOP_NEXTITER || x == JSOP_APPLY ||        \

Comment 16

[Jim Blandy :jimb]

Attachment: suggested change to jstracer.cpp

Bugzilla seems to be mangling that.

Comment 17

[Jonathan Steele]

Attachment: patch 5

Comment 18

[Jim Blandy :jimb]

Comment on attachment 360340 [details] [diff] [review]
patch 5

A stray change crept in to line 3965 of js/src/jstracer.cpp.  That needs to be fixed.

Comment 19

[Jonathan Steele]

Attachment: patch 6

Comment 20

[Jim Blandy :jimb]

Comment on attachment 360342 [details] [diff] [review]
patch 6

Looks good --- thanks!

Comment 21

[Jim Blandy :jimb]

Attachment: Bug 391488: Eliminate PTRDIFF macro; delete jssttddef.h.

[just for reference; this is JonathanS's patch, with the deletion of jsstddef.h included as a git-style patch, and one stray #inclusion of jsstddef.h deleted.]

Delete jsstddef.h, since its only remaining purpose is to make certain
Win16-specific definitions; we don't support Win16 any more.  In
particular, we can just subtract pointers now, so the PTRDIFF macro is
unnecessary noise.

Most places get stddef.h via jstypes.h or some other header, so we can
just delete #inclusions of jstddef.h.  The exception is jskwgen.h, so
there we explicitly include <stddef.h> instead.

Comment 22

[Jim Blandy :jimb]

Landed in M-C:
http://hg.mozilla.org/mozilla-central/rev/00f309d0d507
(congrats, Jonathan!)

Comment 23

[Jonathan Steele]

Is this bugs is fixed?

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> http://mxr.mozilla.org/mozilla-central/search?string=win16&find=/js

I filed bug 482444.