Closed Bug 391488 Opened 12 years ago Closed 11 years ago
Remove all WIN16 code under js directory
31.47 KB, patch
|Details | Diff | Splinter Review|
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
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.
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.
#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.
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.
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 || \
Bugzilla seems to be mangling that.
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 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 ago → 11 years ago
Resolution: --- → FIXED
(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
You need to log in before you can comment on or make changes to this bug.