Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

adds error checking for tf.Buffer.get(outOfRangeLocation) #1630

Merged
merged 2 commits into from Mar 18, 2019
Merged

Conversation

bileschi
Copy link
Contributor

@bileschi bileschi commented Mar 18, 2019

Previously, given a buffer of size [2, 2], accessing [0, 2] and [1, 0] did the same thing, which can be surprising. This PR adds error checking.

Fixes tensorflow/tfjs#1304


This change is Reviewable

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @bileschi and @dsmilkov)


src/tensor.ts, line 101 at r1 (raw file):

      locs = [0];
    }
    for (const i in locs) {

use for..of instead of for..in since the former is for arrays, and the latter is used for iterating through keys of an object, is slower, and doesn't guarantee order.

Copy link
Contributor Author

@bileschi bileschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained


src/tensor.ts, line 101 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

use for..of instead of for..in since the former is for arrays, and the latter is used for iterating through keys of an object, is slower, and doesn't guarantee order.

done.

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@bileschi bileschi merged commit f89def0 into master Mar 18, 2019
@bileschi bileschi deleted the bufferget branch March 18, 2019 16:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants