Last Comment Bug 714728 - Remove jsword?
: Remove jsword?
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla12
Assigned To: :Ms2ger
:
Mentors:
Depends on: 716204
Blocks: 712034
  Show dependency treegraph
 
Reported: 2012-01-03 02:24 PST by :Ms2ger
Modified: 2012-01-11 17:27 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (96.02 KB, patch)
2012-01-04 05:59 PST, :Ms2ger
jwalden+bmo: review+
Details | Diff | Splinter Review

Description :Ms2ger 2012-01-03 02:24:06 PST
jsword and jsuword are just typedefs for intptr_t and uintptr_t, which are already used in js/src. Should we get rid of the indirection?
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 07:14:18 PST
I'm in favor.  I can never remember if jsword is size_t or intptr_t anyway, and I can't be the only one, and every mistake there just makes for more pain for those crazy S/390 people.  :-)
Comment 2 Luke Wagner [:luke] 2012-01-03 08:38:33 PST
While you're on the warpath, can we remove jsint, jsdouble, and every typedef in jstypes.h (except JSBool) and fold the remainder into public/Utility.h?
Comment 3 David Mandelin [:dmandelin] 2012-01-03 15:31:59 PST
Yes!
Comment 4 :Ms2ger 2012-01-04 05:59:42 PST
Created attachment 585720 [details] [diff] [review]
Patch v1
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-06 21:59:03 PST
Comment on attachment 585720 [details] [diff] [review]
Patch v1

Review of attachment 585720 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +3008,4 @@
>  {
>  #if JS_STACK_GROWTH_DIRECTION > 0
>      if (limitAddr == 0)
> +        limitAddr = uintptr_t(-1);

Make this UINTPTR_MAX.

@@ +3021,5 @@
>  #endif
>  
>  #if JS_STACK_GROWTH_DIRECTION > 0
>      if (stackSize == 0) {
> +        cx->stackLimit = uintptr_t(-1);

Ditto.

::: js/src/jscntxt.cpp
@@ +1423,5 @@
>      localeCallbacks(NULL),
>      resolvingList(NULL),
>      generatingError(false),
>  #if JS_STACK_GROWTH_DIRECTION > 0
> +    stackLimit(uintptr_t(-1)),

Ditto.

::: js/src/jsgc.cpp
@@ +1118,5 @@
>       * With 64-bit jsvals on 32-bit systems, we can optimize a bit by
>       * scanning only the payloads.
>       */
>      JS_ASSERT(begin <= end);
> +    for (const uintptr_t *i = begin; i != end; i += sizeof(Value)/sizeof(uintptr_t))

Add spaces around / while you're changing this.

And if I'm pointing out something to change in this line, I might as well request that you change |i != end| to |i < end|.  Trying to avoid potential bad consequences (should something go awry) by making the loop-exit condition still-correct yet broader may be spitting in the wind, when the code in question is part of a garbage collector.  Even still, at the worst it sets a good example.

::: js/src/jslock.cpp
@@ +76,3 @@
>  
>  #if defined(_MSC_VER) && defined(_M_IX86)
>  #pragma warning( disable : 4035 )

I see in passing that this pragma's never undone, so any function in this file that doesn't return a value won't be warned about.  Could you file a bug to use this pragma, and then revert it, in more narrowly-scoped locations?
Comment 6 :Ms2ger 2012-01-07 01:47:05 PST
Comment on attachment 585720 [details] [diff] [review]
Patch v1

>--- a/js/src/jsgc.cpp
>+++ b/js/src/jsgc.cpp
>-MarkRangeConservatively(JSTracer *trc, const jsuword *begin, const jsuword *end)
>+MarkRangeConservatively(JSTracer *trc, const uintptr_t *begin, const uintptr_t *end)
> {
>     JS_ASSERT(begin <= end);
>-    for (const jsuword *i = begin; i != end; ++i)
>+    for (const uintptr_t *i = begin; i != end; ++i)

And if you're requesting that, I might as well make this one consistent.
Comment 8 Nicholas Nethercote [:njn] 2012-01-11 17:27:33 PST
Ms2ger:  JSSize, JSPtrDiff and JSUptrDiff are some additional easy removals if you're in the mood.

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