Add Display trait to Memory struct #812

Jrigada · 2023-02-02T11:05:47Z

Add Display trait to Memory struct

Description

Checklist

Linked to Github Issue
Unit tests added
Integration tests added.
This change requires new documentation.
- Documentation has been added/updated.

fmoletta

Please format the temporary memory too.
Ideally something like

temp_memory: [[1,2,3][1,2]]
real_memory: [[1,2]]

should look like this:

-1:0 : 1
-1:1 : 2
-1:2 : 3
-2:0 : 1
-2:1 : 2
0;0 : 1
0:1 : 2

Oppen · 2023-02-02T13:29:15Z

src/vm/vm_memory/memory.rs

+        for (i, segment) in self.data.iter().enumerate() {
+            for (j, cell) in segment.iter().enumerate() {
+                if let Some(cell) = cell {
+                    writeln!(f, "  {},{} : {}", i, j, cell)?;


Suggested change

writeln!(f, " {},{} : {}", i, j, cell)?;

writeln!(f, " ({i},{j}) : {cell}")?;

fmoletta · 2023-02-09T16:43:54Z

src/vm/vm_memory/memory.rs

@@ -300,6 +301,29 @@ impl Memory {
    }
 }

+impl Display for Memory {
+    fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
+        writeln!(f, "temp_memory {{")?;


We could avoid making distinctions between real and temporary memory and just pritnt them out as a single memory. Temporary segments can be identified by their negative index

fmoletta · 2023-02-09T16:47:57Z

src/vm/vm_memory/memory.rs

+        for (i, segment) in self.temp_data.iter().enumerate() {
+            for (j, cell) in segment.iter().enumerate() {
+                if let Some(cell) = cell {
+                    writeln!(f, "  ({i},{j}) : {cell}")?;


Remember that temporary memory segment indexes start from -1, but we store them from 0 (cuz vec). So when printing out the temporary addresses we should be printing -(i + 1) instead of i

fmoletta · 2023-02-13T13:07:29Z

src/vm/vm_memory/memory.rs

@@ -300,6 +301,28 @@ impl Memory {
    }
 }

+impl Display for Memory {
+    fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
+        writeln!(f, "memory {{")?;


I dont think this is necessary, the user already knows its the memory being displayed if they print the memory

fmoletta · 2023-02-13T13:08:22Z

src/vm/vm_memory/memory.rs

+                }
+            }
+        }
+        writeln!(f, "}}")


These symbols make it look like a Debug rather than a Display, we could avoid them

codecov · 2023-02-13T17:48:43Z

Codecov Report

Merging #812 (9d9aab1) into main (4a642c7) will increase coverage by 97.81%.
The diff coverage is 93.33%.

@@            Coverage Diff            @@
##           main     #812       +/-   ##
=========================================
+ Coverage      0   97.81%   +97.81%     
=========================================
  Files         0       69       +69     
  Lines         0    29410    +29410     
=========================================
+ Hits          0    28767    +28767     
- Misses        0      643      +643

Impacted Files	Coverage Δ
src/vm/vm_memory/memory.rs	`99.18% <93.33%> (ø)`
src/types/layout.rs	`99.50% <0.00%> (ø)`
src/types/relocatable.rs	`99.31% <0.00%> (ø)`
src/vm/runners/builtin_runner/hash.rs	`99.06% <0.00%> (ø)`
...or/builtin_hint_processor/skip_next_instruction.rs	`100.00% <0.00%> (ø)`
...uiltin_hint_processor/cairo_keccak/keccak_hints.rs	`95.37% <0.00%> (ø)`
...ocessor/builtin_hint_processor/secp/field_utils.rs	`100.00% <0.00%> (ø)`
src/vm/runners/builtin_runner/signature.rs	`97.58% <0.00%> (ø)`
...t_processor/builtin_hint_processor/sha256_utils.rs	`100.00% <0.00%> (ø)`
src/vm/runners/builtin_runner/bitwise.rs	`96.73% <0.00%> (ø)`
... and 60 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

fmoletta

Nice!

add test for display memory

f96d53f

Jrigada requested review from igaray, Oppen, fmoletta, entropidelic, HermanObst, juanbono, pefontana and Juan-M-V as code owners February 2, 2023 11:05

Jrigada and others added 2 commits February 2, 2023 13:14

Merge branch 'main' into add-display-trait-to-memory

230d632

cargo fmt

999f5e7

fmoletta reviewed Feb 2, 2023

View reviewed changes

Oppen reviewed Feb 2, 2023

View reviewed changes

display temporary segments

f9d82bc

Jrigada requested review from Oppen and fmoletta February 9, 2023 12:59

fmoletta reviewed Feb 9, 2023

View reviewed changes

display whole memory without distinction and fix segment indexes

20e7a37

Jrigada requested a review from fmoletta February 13, 2023 11:42

fmoletta reviewed Feb 13, 2023

View reviewed changes

Oppen approved these changes Feb 13, 2023

View reviewed changes

remove memory text from display of memory

9d9aab1

Oppen approved these changes Feb 13, 2023

View reviewed changes

fmoletta approved these changes Feb 13, 2023

View reviewed changes

fmoletta added this pull request to the merge queue Feb 13, 2023

Merged via the queue into main with commit 0991fb4 Feb 13, 2023

fmoletta deleted the add-display-trait-to-memory branch February 13, 2023 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Display trait to Memory struct #812

Add Display trait to Memory struct #812

Jrigada commented Feb 2, 2023

fmoletta left a comment •

edited

Oppen Feb 2, 2023

fmoletta Feb 9, 2023

fmoletta Feb 9, 2023

fmoletta Feb 13, 2023

fmoletta Feb 13, 2023

codecov bot commented Feb 13, 2023 •

edited

fmoletta left a comment

	writeln!(f, " {},{} : {}", i, j, cell)?;
	writeln!(f, " ({i},{j}) : {cell}")?;

Add Display trait to Memory struct #812

Add Display trait to Memory struct #812

Conversation

Jrigada commented Feb 2, 2023

Add Display trait to Memory struct

Description

Checklist

fmoletta left a comment • edited

Choose a reason for hiding this comment

Oppen Feb 2, 2023

Choose a reason for hiding this comment

fmoletta Feb 9, 2023

Choose a reason for hiding this comment

fmoletta Feb 9, 2023

Choose a reason for hiding this comment

fmoletta Feb 13, 2023

Choose a reason for hiding this comment

fmoletta Feb 13, 2023

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2023 • edited

Codecov Report

fmoletta left a comment

Choose a reason for hiding this comment

fmoletta left a comment •

edited

codecov bot commented Feb 13, 2023 •

edited