Closed Bug 788403 Opened 10 years ago Closed 10 years ago

ParallelArray.scatter iterates incorrect number of times and incorrect row bounds checking

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: shu, Assigned: shu)

Details

(Whiteboard: [js:t])

Attachments

(1 file)

In retrospect, the fix in bug 787667 is a bandaid; the scatter operation ultimately still iterates an incorrect number of times.

Furthermore, not all elements on ParallelArrays whose scalar length (the product of all its dimensions) should be undefined, as the bandaid in bug 787667 ended up causing. For instance,

  var p = new ParallelArray([2,0], function f() { return 0; });
  p[0] // should be an empty ParallelArray, not undefined
Assignee: general → shu
Flags: in-testsuite+
Attachment #658379 - Flags: review?(dmandelin)
Comment on attachment 658379 [details] [diff] [review]
fix and testcases

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

Pretty straightforward. I found a couple of things but no need to run through another cycle with the requested comment dele and as long as the possible can't-happen in inBounds() actually isn't.

::: js/src/builtin/ParallelArray-inl.h
@@ +18,5 @@
> +ParallelArrayObject::IndexInfo::inBounds() const
> +{
> +    JS_ASSERT(isInitialized());
> +
> +    if (indices.length() > dimensions.length())

I thought indices.length() <= dimensions.length() was an invariant of IndexInfo. Is that incorrect? If it is an invariant, then this should be an assertion instead.

::: js/src/builtin/ParallelArray.h
@@ +118,5 @@
>          // Load dimensions from a source, then initialize as above.
>          inline bool initialize(JSContext *cx, HandleParallelArrayObject source,
>                                 uint32_t space);
>  
> +        // Is the index in bounds?

This comment should be either more or less. As it stands, it's redundant with the name. In this case, I would say it's better to have no comment, because it's part of an internal API, it's pretty obvious what it does from the name, and if anyone wants to know more, the source of the function is very easy to read.
Attachment #658379 - Flags: review?(dmandelin) → review+
Whiteboard: [js:t]
https://hg.mozilla.org/mozilla-central/rev/0a941f6df7f6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.