Skip to content

feat: array memory mapping#20

Merged
baszalmstra merged 28 commits intobaszalmstra:feature/arraysfrom
Wodann:refactor/memory-mapping
Nov 13, 2022
Merged

feat: array memory mapping#20
baszalmstra merged 28 commits intobaszalmstra:feature/arraysfrom
Wodann:refactor/memory-mapping

Conversation

@Wodann
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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?

);
}

#[test]
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same

Copy link
Copy Markdown
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