Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arrays of anything #434

Merged
merged 44 commits into from
Sep 23, 2019
Merged

Arrays of anything #434

merged 44 commits into from
Sep 23, 2019

Conversation

Schaeff
Copy link
Member

@Schaeff Schaeff commented Jul 16, 2019

extension of #402 to any array content

zokrates_core/src/lib.rs Outdated Show resolved Hide resolved
@stefandeml stefandeml self-assigned this Sep 12, 2019
res.into_iter().map(|r| r.into()).collect()
}

/// Flatten a array selection expression
Copy link
Member

Choose a reason for hiding this comment

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

"an", if you wanna be nitpicky ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

wanna try the github "suggest change" feature? :p

///
/// # Arguments
///
/// * `symbols` - Available functions in in this context
Copy link
Member

Choose a reason for hiding this comment

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

remove one "in"

///
/// # Arguments
///
/// * `symbols` - Available functions in in this context
/// * `statements_flattened` - Vector where new flattened statements can be added.
/// * `expression` - `FieldElementArrayExpression` that will be flattened.
fn flatten_field_array_expression(
/// * `expression` - `ArrayExpression` that will be flattened.
Copy link
Member

Choose a reason for hiding this comment

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

should be 'expr'

@stefandeml
Copy link
Member

Just to persist this here as well:

My main take on that PR for now is that
a) we need update the docs with the new features added to the DSL
b) make sure that we have more tests written in DSL for these features, like we do in the stdlib. However to me this should be done in a separate PR.

So after fixing the docs and potentially fixing some of the typos, this should be merged.

@JacobEberhardt
Copy link
Member

In the current version, how we process indices of multidimensional array declarations is unlike in C/Java. See the following example:

def main() -> (field):
    // valid ZoKrates
    field[3][2] a = [[1, 2, 3],[4, 5, 6]]
    // Interpretation array of three elements packed in array of two elements

    // allowed access a[0..2][0..3] -> not [0..3][0..2] as you may expect from declaration

    // not working:
    // C/Java world: Array of two elements of array of 3 elements
    field[2][3] b = [[1, 2, 3],[4, 5, 6]]
    b[1] // should be [1, 2, 3]


    // allowed access [0..2][0..3]
    return a[1][2]

Beyond being "established", the C/Java version is also aligned with the intuition of declaring and manipulating matrices. Hence, I would propose to change the implementation to mirror that behaviour.

@JacobEberhardt JacobEberhardt self-assigned this Sep 17, 2019
@Schaeff Schaeff merged commit 04fe7ef into develop Sep 23, 2019
@Schaeff Schaeff mentioned this pull request Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants