Skip to content

feat: array memory mapping #20

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

Merged
merged 28 commits into from
Nov 13, 2022

Conversation

Wodann
Copy link

@Wodann Wodann commented Aug 4, 2022

Creates a clearer separation between the calculation of a memory mapping and the execution thereof. Also, adds support for array memory mapping.

Supported array mappings (recursively) are:

[f64] <-> f64 (array from/to primitive + valid casts)
[f32] <-> [[f64]] (array from/to primitive + valid casts)
[Foo] <-> Foo (array from/to struct + struct mapping)

@Wodann Wodann changed the title WIP: refactor: memory mapping refactor: memory mapping Aug 15, 2022
@Wodann Wodann requested a review from baszalmstra August 15, 2022 03:28
Copy link
Owner

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

This already looks so much cleaner than the previous implementation. Great job!

pub struct Foo {
a: i32,
b: [Bar],
c: [Baz],
Copy link
Owner

Choose a reason for hiding this comment

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

Can this code be removed?

@@ -637,6 +637,1423 @@ fn map_struct_all() {
);
}

#[test]
Copy link
Owner

Choose a reason for hiding this comment

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

All of these tests, test if the value of a certain instance has not changed. However, this is done through get which also performs casting if possible. I think besides testing whether or not the content changes, it should also test whether or not the type actually changes.

Copy link
Owner

Choose a reason for hiding this comment

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

The tests also feel very verbose to me. Although its a good thing because they thoroughly test the code it also makes them much harder to maintain if we change details down the line. Im wondering if its possible to reduce the number of lines required to setup a test like this... Its just a thought.

baszalmstra and others added 9 commits August 27, 2022 23:09
…-lang#447)

Updates the requirements on ra_ap_text_edit to permit the latest version.

---
updated-dependencies:
- dependency-name: ra_ap_text_edit
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…-lang#449)

Updates the requirements on ra_ap_text_edit to permit the latest version.

---
updated-dependencies:
- dependency-name: ra_ap_text_edit
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updates the requirements on [criterion](https://github.com/bheisler/criterion.rs) to permit the latest version.
- [Release notes](https://github.com/bheisler/criterion.rs/releases)
- [Changelog](https://github.com/bheisler/criterion.rs/blob/master/CHANGELOG.md)
- [Commits](bheisler/criterion.rs@0.3.0...0.4.0)

---
updated-dependencies:
- dependency-name: criterion
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updates the requirements on [lockfile](https://github.com/derekdreery/lockfile-rs) to permit the latest version.
- [Release notes](https://github.com/derekdreery/lockfile-rs/releases)
- [Changelog](https://github.com/derekdreery/lockfile-rs/blob/master/CHANGELOG.md)
- [Commits](https://github.com/derekdreery/lockfile-rs/commits)

---
updated-dependencies:
- dependency-name: lockfile
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…-lang#451)

Updates the requirements on ra_ap_text_edit to permit the latest version.

---
updated-dependencies:
- dependency-name: ra_ap_text_edit
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…-lang#460)

Updates the requirements on ra_ap_text_edit to permit the latest version.

---
updated-dependencies:
- dependency-name: ra_ap_text_edit
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…-lang#473)

Updates the requirements on ra_ap_text_edit to permit the latest version.

---
updated-dependencies:
- dependency-name: ra_ap_text_edit
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
used_deletions
.into_iter()
.zip(deletions.into_iter())
.for_each(|(used, (_, old_index, ty, _))| {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, I would write this as a regular for statement.

Copy link
Author

Choose a reason for hiding this comment

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

I actually like these as you're already explicitly working with zipped iterators. I changed the other one

used_insertions
.into_iter()
.zip(insertions.into_iter())
.for_each(|(used, (_, new_index, ty, _))| {
Copy link
Owner

Choose a reason for hiding this comment

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

Same

Copy link
Author

Choose a reason for hiding this comment

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

Also didn't resolve for same reason as above comment

@Wodann Wodann changed the title refactor: memory mapping feat: array memory mapping Nov 12, 2022
@baszalmstra baszalmstra merged commit 7f7600d into baszalmstra:feature/arrays Nov 13, 2022
@Wodann Wodann deleted the refactor/memory-mapping branch November 14, 2022 03:47
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.

2 participants