Closed Bug 391488 Opened 12 years ago Closed 11 years ago

Remove all WIN16 code under js directory

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: xfsunoles, Assigned: xfsunoles)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 8 obsolete files)

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.
Summary: Remove all WIN16 code → Remove all WIN16 code in jsstddef
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---
What about </js2/src/*>, </js/jsd/*>, </js/src/*> ?
Version: unspecified → Trunk
Summary: Remove all WIN16 code in jsstddef → Remove all WIN16 code under js directory
All <js*>:
http://mxr.mozilla.org/mozilla-central/search?string=win16&find=/js

<js2> exists in cvs but not in Hg.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Tell it so (obsolete) — Splinter Review
Untestes
Attachment #353942 - Flags: superreview?(brendan)
Attachment #353942 - Flags: review?(brendan)
Attachment #353942 - Flags: superreview?(brendan)
Attachment #353942 - Flags: review?(jim)
Attachment #353942 - Flags: review?(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
BTW, no super-review for SpiderMonkey.

/be
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.
Attachment #353942 - Flags: review?(jim) → review-
Typo: that second bug is bug 269538.
Attached patch Remove PTRDIFF (obsolete) — Splinter Review
I did remove all prtdiff defined with jsstddef.h too. I didn't remove other js files that just have jsstddef.h.
Assignee: general → xfsunoles
Attachment #353942 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
Attached patch part 2 (obsolete) — Splinter Review
#1,2 and 3 are done. I look at #4 later.
Attachment #359113 - Attachment is obsolete: true
(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.
Attached patch part 3 (obsolete) — Splinter Review
I added part 2 with #include <stddef.h> in jskwgen.cpp.
Attachment #359142 - Attachment is obsolete: true
Part 3 looks good, just need that point #4 taken care of.
Attached patch patch 4 (obsolete) — Splinter Review
I hope this patch is exactly you wants. I did move it to own line.
Attachment #359323 - Attachment is obsolete: true
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 ||        \
Attached patch suggested change to jstracer.cpp (obsolete) — Splinter Review
Bugzilla seems to be mangling that.
Attached patch patch 5 (obsolete) — Splinter Review
Attachment #359589 - Attachment is obsolete: true
Attachment #359596 - Attachment is obsolete: true
Attachment #360340 - Flags: review?(jim)
Attachment #360340 - Flags: review?(jim) → review-
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.
Attached patch patch 6 (obsolete) — Splinter Review
Attachment #360340 - Attachment is obsolete: true
Attachment #360342 - Flags: review?(jim)
Comment on attachment 360342 [details] [diff] [review]
patch 6

Looks good --- thanks!
Attachment #360342 - Flags: review?(jim) → review+
[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.
Attachment #360342 - Attachment is obsolete: true
Landed in M-C:
http://hg.mozilla.org/mozilla-central/rev/00f309d0d507
(congrats, Jonathan!)
Is this bugs is fixed?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
Depends on: 482444
(In reply to comment #2)
> http://mxr.mozilla.org/mozilla-central/search?string=win16&find=/js

I filed bug 482444.
Target Milestone: --- → mozilla1.9.2a1
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.