Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Depends on: 1 bug)

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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?
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

6 years ago
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?
(Assignee)

Updated

6 years ago
Blocks: 712034
Yes!
(Assignee)

Comment 4

6 years ago
Created attachment 585720 [details] [diff] [review]
Patch v1
Assignee: general → Ms2ger
Status: NEW → ASSIGNED
Attachment #585720 - Flags: review?(jwalden+bmo)
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?
Attachment #585720 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Updated

6 years ago
Depends on: 716204
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/2f310f456107
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Ms2ger:  JSSize, JSPtrDiff and JSUptrDiff are some additional easy removals if you're in the mood.
You need to log in before you can comment on or make changes to this bug.